Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint visible documents on activation #141

Conversation

marcusrbrown
Copy link
Contributor

Changes the extension activation to request linting of all visible documents, not just the active one.

I noticed a small bug while working on the extension where a visible but inactive Markdown file would not be linted until either:

  1. A change was made to the document or it was saved.
  2. The Output tab was focused. The assumption is that this causes the onDidChangeVisibleTextEditors event to fire.

Copy link
Owner

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review 33c2e4a. I made that change back when I was occasionally getting "took too long to initialize" bugs. What eventually fixed that was bundling, so I'm open to undoing that optimization - especially in light of the bug you report. However, it looks like you change might be more simply implemented with a call to cleanLintVisibleFiles?

@marcusrbrown marcusrbrown force-pushed the fix/lint-visible-documents-on-activation branch from 041dc52 to 17ca464 Compare November 29, 2020 01:53
@marcusrbrown
Copy link
Contributor Author

Thanks for the clarification. I didn't use cleanLintVisibleFiles because it calls didChangeVisibleTextEditors which uses lint instead of requestLint. Thinking about it, that isn't necessarily an optimization unless requestLint and suppressLint are modified to support multiple documents. That itself may be an over-optimization unless a lint is pending over multiple documents that are being closed or modified via a replace action?

Updated to use cleanLintVisibleFiles during activation.

@DavidAnson
Copy link
Owner

You make a great point. I definitely do not want to add a synchronous lint of N documents to the load path. But as you say, requestLint isn't set up for multiple simultaneous calls. What about just wrapping the new call in a setTimeout using the same delay value?

@marcusrbrown marcusrbrown force-pushed the fix/lint-visible-documents-on-activation branch from 17ca464 to d807440 Compare November 29, 2020 04:02
@marcusrbrown
Copy link
Contributor Author

Updated to wrap the call to cleanLintVisibleFiles with setTimeout in d807440.

extension.js Outdated
requestLint(vscode.window.activeTextEditor.document);
}
// Lint all visible documents
setTimeout(() => cleanLintVisibleFiles(), throttleDuration);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a function that calls cLVF, or can we pass it directly by name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ba0da26.

It would be great to have a global ESLint rule similar to Lodash's no-extraneous-function-wrapping available.

@marcusrbrown marcusrbrown force-pushed the fix/lint-visible-documents-on-activation branch from d807440 to ba0da26 Compare November 29, 2020 21:44
Copy link
Owner

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@DavidAnson DavidAnson merged commit d155351 into DavidAnson:next Nov 29, 2020
@marcusrbrown marcusrbrown deleted the fix/lint-visible-documents-on-activation branch November 29, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants