-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix #6661. Configurable multiple linters. #7362
Conversation
I wonder if this should be done via the LanguageLayer |
providers = installedProviders; | ||
} | ||
if (langPref.firstOnly === true) { // implies suppressing others | ||
return providers.slice(0, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work with prefer = null
and firstOnly = true
or sth like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, that's a good point -- i don't think i've added a test case for that, but it should work. when prefer
is falsy, list of installed providers will use used (line 180), and then only the first will be picked. i'll add a test case for this (and fix if it does not work), thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. But a pretty uncommon combination could still break:
prefer = [unavailable provider]
(this could cause problems either way...)
preferredOnly = true
(so providers
is empty)
firstOnly = true
(split on an empty object)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This one needs a test and a fix too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, both work. Just added testcases. Thank you for the idea.
What's the difference between a preferred provider and the others (if |
You can register multiple inspector for the same language, but if you specify Currently we have this special handling for javascript inspectors. Once you register a new javascript inspector, the |
I'd say, we should split the work and first re-implement the LanguageLayer in a separate PR. When it's done, you can merge the changes and do work on this PR. |
Ok. Makes sense, there's time. This one should go after async linting anyways. @dangoor, I'll assign resurrection of LanguageLayer to myself then, OK? |
@busykai Sounds good, thanks! |
Test that non-existing provider, preferredOnly = true and firstOnly = true.
Also, change single-quote to double-quotes for string literals.
@busykai What is the status of this PR? What needs to be done to finish this PR? Thanks! |
@ingorichter, it depends on language layer which i'm planning to finish up this week and re-work this one to use it. we can close it so it wouldn't bother (but please don't delete the branch). |
@busykai I'd like to get this feature into Brackets soon. This would make things so much easier to use for me (working on projects that mainly use JSHint instead of JSLint). I don't always want to install und uninstall JSHint before working on Brackets again. I'm afraid that once we close this PR, we will forget about it. ;-) |
@ingorichter, yes. I do want the same thing. We'll make it to Release 42. |
…i-linters Conflicts: src/language/CodeInspection.js test/spec/CodeInspection-test.js
…nto kai/fix-6661-multi-linters
…nto kai/fix-6661-multi-linters
…nto kai/fix-6661-multi-linters
Fix issues with logic.
Remove corresponding tests. Clean up tests.
@TomMalbran Yeah that's my understanding as well. I just think that being explicit about what you want will lead to cleaner/clearer configurations. I would prefer to just write |
I also prefer @MiguelCastillo's explicit definition, if defining at all. When not specifying anything (not giving a |
@TomMalbran, good idea. It could be built on top of this on top of this though. For the most cases, provider is an extension and can be uninstalled. JSLint is special. Now that we have pre-installed extension, it should probably be done as such as opposed to being a default one. Out of scope of this PR, of course.
@MiguelCastillo, The only case not covered by this one preference scenario is installing a new provider. If we had just You, in turn, can accomplish the desired behaviour by using |
@busykai So your use case of a newly installed extension cleared up for me why you are using the flag. The only thing that the flag does not cover is order in cases where there is more than one provider that is not in the list. I can actually work on putting together an extension to provide a UI to configure this stuff. I know @Mark-Simulacrum would help me :D Right buddy? It would be a simple dialog listing all providers with a simple checkbox to toggle them on and off... Maybe group them by language. I am not sure about ordering though; dragging would be a nice fancy way to do it, but we can add that later. |
Yes I was thinking of it as a new preference, with lower priority to prefer. So if I have
You can only do that if you intend to use the preference just for sorting, which I don't think that will be the main use for this preference. So if you set |
I would really like to come to a decision on this. I think we can stick to current two-preference set without any harm being done going forward (all the ideas could be done as additional settings). Let's help @ingorichter decide. @TomMalbran, @MiguelCastillo would you agree this PR is 1) mergable, 2) serves its purpose and 3) you don't see something fundamentally flawed that cannot be be extended later on? Simple yes/no does it. |
I am good with this. Putting a UI in front of this will definitely help users having to understand the details behind the |
|
@TomMalbran, in order to help the decision, you need to mention "other people" whose opinion you'd like to hear to invite them and express it. Would you agree? |
Yes. It would be nice to hear if others agree with this preferences or prefer others, since I am still not convinced. The current preferences are fine, but maybe we can improve them. |
Updated PR description. Here is the modified table based on @ingorichter's above Preferences
|
I'm glad to see you all improving this feature with your questions, ideas and the constructive feedback. IMHO, this is a great improvement over the existing feature and will give our users control over which linting providers will be used. Even if we don't feel that this is a perfect solution right now, let's get it merged into master and iterate on it when necessary. |
Fix #6661. Configurable multiple linters.
Yay! No more suffering over multiple projects with different linters! And less "Fix JSHint error" in Brackets codebase! 👍 |
Make JSLint believe it's alone.
Fix failing JSLint unit test after #7362.
I'm having issues with this change. After upgrading to 1.1, all Javascript files only execute JSLint, instead of the other linter extensions I have installed. I put this into my preferences:
But it seems to have no effect. Is there a page documenting this change, and how I can configure my preferences to return Brackets to its 1.0 behavior (disabling JSLint and using custom linting extensions)? |
@globexdesigns Notice that you need to use an array for the prefer preference:
|
@TomMalbran beat to it :) The problem seems to be with |
I guess that we should update the description of this issue which doesn't use the final preferences. |
That did it. Thanks @TomMalbran . It would still be nice to document this somewhere as I expect many others will come here with the same question. Especially since this update changes default behaviour. |
It is documented and an example is provided. |
Something I started at the hackathon and finished offline.
This PR fixes #6661. It introduces an option to control execution of linters through a configuration file. It uses
LanguageLayer
implemented in #7889 (#7889 should be merged first). No UI impact.For any given language
languageID
, the following set of options can be defined:linting.prefer
is the list of code inspection providers (does not matter whether they are installed or not, does not matter if it is complete or not). ifpreferredOnly
is specified, then only providers fromprefer
list will be used (those which are installed, of course).preferredOnly
will have no effect ifprefer
is not specified. Test cases are provided which are probably the best documentation.One could think of
prefer
as "the list of providers I would like to see first" andusePreferredOnly
as of "I would only like to see the providers I prefer".For example, the following configuration is used in this PR for Brackets:
Which would enable both
JSLint
andJSHint
(they will be used in that order).See #6661, #6662
CC: @dangoor, @peterflynn, @ingorichter