CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Change CodeInspection Public API to expose providers #9189
Change CodeInspection Public API to expose providers #9189
Conversation
Changes getProvidersForPath to be public - allows extensions to query providers for custom linting. Adds getProviders to allow extensions to get all providers. Adds deregister to allow extensions to remove a provider when the extension provides a provider with the same functionality.
* | ||
* Returns false if no provider was removed. | ||
*/ | ||
function deregister(languageId, provider) { |
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.
unregister
is the opposite of register
done with first pass review. this pr needs unit tests for the new APIs. |
Change arguments of unregister() to not require a full provider object, just the name. Return true if any providers are removed in unregister().
@@ -469,6 +469,30 @@ define(function (require, exports, module) { | |||
} | |||
|
|||
/** | |||
* Deregisters a provider, given a languageId and the provider. |
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.
Nit: 'Unregister' seems more readable
I think there is an important aspect to PR that should be mentioned... All we are looking for is a way to get linting reports from linters registered in code inspector. We could do this in a couple of different ways, and one is having interactive linter, or any other extension man handle the linters directly... Another option is to provide the means for extensions to register a callbacks to get a copy of the report anytime a linter runs. I personally feel that allowing extensions to unregister linters is not a good idea. We can instead provide a way to specify priorities. Very similar to how we do it in the CodeHintManager https://github.com/adobe/brackets/blob/master/src/editor/CodeHintManager.js#L286 |
I like @MiguelCastillo's idea of prioritizing providers for CodeInspection - but one issue I see is that other extensions won't know what to set the priority to if they want to "override" all other extensions - and we could have a case of 2 extensions wanting the highest priority (and not getting it). |
@Mark-Simulacrum Yeah that's a fair point. But just like the code hinting priorities, you normally dont have multiple extensions registered to do that same job. That is to say, you normally don't have multiple CSS linting extensions or multiple JSHint extensions registered to compete for priority. And on that same token, any extension could |
I guess that is true. Although I feel like the whole point behind priorities is "multiple extensions doing the same job" - which is the argument against |
@Mark-Simulacrum The issue is that I don't feel right allowing extensions to unregister/remove other extensions... Just feels really dirty and unsafe. |
I feel like moving to priorities is a major architectural change for What do you think? |
@Mark-Simulacrum I think that's probably a much better approach. |
I've just pushed another commit (5ffdc97) that makes this PR only make |
@Mark-Simulacrum What you have done is good. You can easily go overboard with unit testing... Like you could add a unit for getting linters when you pass a language for which no linters have been registered. You can add another one for when the linter you are registering is invalid (pass in a null), and you try to get it. Registering linters with different casing... You can never have too many unit tests. So, what you just have to think about is if you have enough tests in place so that if you made a change in the future, would your unit tests catch your mistakes. I am really bad with unit tests... So we need some more feedback here. |
One thing I'd like to point out is that currently, we return undefined if the language has no linters - perhaps we should return an empty array instead? |
That seems like a good thing. Since |
@Mark-Simulacrum please look at the #7362. Some of the issues (like provider prioritization are addressed by it). |
FYI I'm currently looking at #7889 in order to get this PR in. |
Changed title to better fit the (current) goals of this PR. |
… registered for language.
}); | ||
|
||
it("should return an empty array if no providers are registered", function () { | ||
expect(CodeInspection.getProvidersForPath("test.js").length).toBe(0); |
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.
JSHint complains about that line. (See https://travis-ci.org/adobe/brackets/builds/37012273)
Ready for another review. |
@Mark-Simulacrum, @MiguelCastillo, I actually feel (just my opinion) that this PR and related discussion that started it try to solve the problem which has to be solved in core by adding "native" interactive linting. There is a Trello card for that. How about upvoting it as well? |
@@ -161,7 +161,7 @@ define(function (require, exports, module) { | |||
* @return ?{Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}>} provider | |||
*/ | |||
function getProvidersForPath(filePath) { | |||
return _providers[LanguageManager.getLanguageForPath(filePath).getId()]; | |||
return _providers[LanguageManager.getLanguageForPath(filePath).getId()] || []; |
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 should return a clone of the array since it will be possible to modify the original _providers
by, for example, using Array.prototype.pop()
.
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.
Done.
…e modification.
@MiguelCastillo @busykai While interactive linting might be a feature that should get included in core, I think that the changes in this PR (exposing registered providers given a file path) should still be included in the core (and exposed) because extensions that want to implement other things, like selective enabling of linters, would need them in order to implement their functionality. |
@busykai Not even sure why I never upvoted it before... I just did :) |
@ingorichter Any objections to merging this in? |
@MiguelCastillo Good point. No, I'm okay with merging this. |
…ccess Change CodeInspection Public API to expose providers
@ingorichter thanks!! |
@ingorichter Found a bug in my own code, have put up a fix here: #10068. |
Although this changes the public API, it does not change any existing methods - it just expands upon them. For this reason, I think that this should not produce any backwards compatibility issues.
Some discussion on this topic has been done here: MiguelCastillo/Brackets-InteractiveLinter#44.
/cc @MiguelCastillo