Change CodeInspection Public API to expose providers

[Mark-Simulacrum]

• 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.

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

[JeffryBooher]

unregister is the opposite of register

[JeffryBooher]

done with first pass review. this pr needs unit tests for the new APIs.

[MiguelCastillo]

Nit: 'Unregister' seems more readable

[MiguelCastillo]

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

[Mark-Simulacrum]

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).

[MiguelCastillo]

@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 unregister other linting extensions so you are running into the same dilemma as if you were competing for priority.

[Mark-Simulacrum]

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 unregister, as far as I can tell (That multiple extensions could want to register themselves over others).

[MiguelCastillo]

@Mark-Simulacrum The issue is that I don't feel right allowing extensions to unregister/remove other extensions... Just feels really dirty and unsafe.

[Mark-Simulacrum]

I feel like moving to priorities is a major architectural change for CodeInspection (and will break many extensions). Perhaps we should consider removing the unregister method from this PR - and just leaving the public access to getProvidersForPath. That should be enough for most extensions (such as Interactive Linter) to lint files. I'm willing to work on changing the API to use priorities instead of a push statement into a provider array, but I feel like this PR is something that would be helpful to extensions (such as InteractiveLinter), without remove/override provider functionality.

What do you think?

[MiguelCastillo]

@Mark-Simulacrum I think that's probably a much better approach.
@JeffryBooher what do you think?

[Mark-Simulacrum]

I've just pushed another commit (5ffdc97) that makes this PR only make getProvidersForPath public, and add a unit test for getProvidersForPath. I'd like someone to check my unit test - I'm not sure if what I've done there is proper for unit tests (I've only written unit tests once - in my other PR for Brackets: #9190).

[MiguelCastillo]

@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.

cc @dangoor @peterflynn

[Mark-Simulacrum]

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?

[MiguelCastillo]

That seems like a good thing. Since getProvidersForPath is currently private, we should be able to quickly verify and correct anywhere that's checking for undefined. Adjust unit tests as well.

[busykai]

@Mark-Simulacrum please look at the #7362. Some of the issues (like provider prioritization are addressed by it).

cc @MiguelCastillo

[ingorichter]

FYI I'm currently looking at #7889 in order to get this PR in.

[Mark-Simulacrum]

Changed title to better fit the (current) goals of this PR.

[marcelgerber]

JSHint complains about that line. (See https://travis-ci.org/adobe/brackets/builds/37012273)

[Mark-Simulacrum]

Ready for another review.

[busykai]

@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?

[busykai]

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().

[Mark-Simulacrum]

Done.

[Mark-Simulacrum]

@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.

[MiguelCastillo]

@busykai Not even sure why I never upvoted it before... I just did :)
@ingorichter This change seems like a pretty isolated change that isn't really doing much with the language layer. Is there a reason we can't merge this one in now?

[MiguelCastillo]

@ingorichter Any objections to merging this in?

[ingorichter]

@MiguelCastillo Good point. No, I'm okay with merging this.

[MiguelCastillo]

@ingorichter thanks!!

[Mark-Simulacrum]

@ingorichter Found a bug in my own code, have put up a fix here: #10068.