Pluggable linting #4588

peterflynn · 2013-07-27T00:07:40Z

This moves the lint panel / status bar UI back into core but makes is generic. Extensions can register linting providers per language, and the core code manages invoking the linter at the right time and presenting the results in the UI.

The panel header and tooltip strings reflect the name of whatever linter is active for the current file. I split the "disabled" tooltip to discriminate between globally disabled vs. no linter available for a speific filetype.

One nice thing about having all linter extensions headlessly bottleneck though a core API is that it lets us easily add future linting features that most extensions will get "for free." See the header comment in Linter.js for some ideas.

Here's an example of how simply an extension can provide a new linter: https://gist.github.com/peterflynn/6116248 (CSSLint in this case)

core) and headless JSLint-specific 'linting provider' (in extension)

* Separate out "JSLint" strings, so linter name is also extensible * Nicer-structured linting results API; JSLint extension translates JSLint results into this generic format * Different tooltips for linting disabled vs. no linter for filetype

* Remove "jslint" references from CSS/HTML names * Add linting levels to linting API (not exposed in UI yet) * Fix bug: exception clicking in empty parts of errors panel * Fix bug: panel title blank if was closed at startup time

about decrementing the error total. * Remove "JSLint" from toggle-enabled command * Fix bug where tooltip string for the disabled case wasn't used

peterflynn · 2013-07-27T00:09:22Z

Btw, I'm on the fence about whether to continue calling this (at the API level) "linting" or something more generic like "errors/warnings." The latter makes sense if we think we'd eventually consolidate errors from manually-invoked / full-project tools (e.g. Closure, or Ant/Grunt) into the same UI panel.

pthiess · 2013-07-27T00:30:30Z

Cool! :) A Code Quality Scanner API ...

TomMalbran · 2013-07-27T01:37:29Z

Nice! Two things thought:

I think that each linter should be able to be enabled or disabled separately. I could then easily have JSLint enabled and CSSLint disabled if this is what I wanted. Maybe the collapsible panel too. With the new extensions APIs these preferences can be deleted when a linter is uninstalled.
A language could easily have multiple linters, like having both JSLint and JSHint installed. Then each linter should have a priority and then only the one with the highest one will run. If we can disable each linter separately, we could then disable the linter with the highest priority and the next one will run. An alternative, would be to run both linters, but we might need to have tabbed panels to properly show the errors.

ingorichter · 2013-07-29T19:22:09Z

I think the API name should be more generic, since I can think of other tools to apply to the source (metrics, duplicate code detector) that aren't linter per se. I don't have a good name right now. Perhaps it should be named CodeQualityReporter, ProblemReporter.
I like the idea of calling the UI Panel "Problem View" then. Error sounds too harsh, problem sounds serious, but not too serious.

njx · 2013-07-29T19:41:36Z

I had the same feedback as @ingorichter - it seems to me like there are various kinds of reporting that this UI could support that aren't errors. How about something like CodeAnalyzerView?

peterflynn · 2013-07-30T19:42:28Z

@TomMalbran: I think we should definitely have more UI around this in the future, but I wanted to keep it minimal for now in the interest of getting this landed sooner. I think it's reasonable as a first pass to assume that if the user wants CSS linting they'll install a CSS linting extension, and if they don't want CSS linting they won't install it -- so individual disabling isn't needed. JS linting is already built into core, but can be disabled via the existing global toggle. And this code allows extensions to overwrite the default core JS linter with a different one.

@njx @ingorichter: I could see naming it CodeAnalysis or CodeInspection... though eventually the same "Problems panel" area will show build errors too, which aren't really analysis/inspection. At that point perhaps we'd keep the "CodeInspection" API/module around for the sake of managing linters, but the panel ownership would move to a new, more generic module that could also be talked to by the build tools feature?

peterflynn · 2013-07-30T19:46:24Z

Btw, here's an example of how easily other linters can be provided by an extension:
https://gist.github.com/peterflynn/6116248 (CSSLint in this case)
[added to notes at top]

TomMalbran · 2013-07-30T20:23:20Z

@peterflynn Sure, we probably would need submenus or a proper preferences panel to add all the linters options. Which could even be added automatically when registering a linter (and removed when un-registering).

peterflynn · 2013-07-30T23:49:47Z

@TomMalbran I actually think we could get away with doing a lot of the linter selection/enablement with simple UI in the results panel itself, e.g. the linter name at top could be a dropdown picker. Settings specific to each linter type (e.g. the stuff in JSLint header comments or .jshintrc files) are a whole other can of worms though :-)

TomMalbran · 2013-07-30T23:59:26Z

@peterflynn Nice idea, but how could you re-enable a linter when there are no results for it, since it is disabled? Unless your idea is to select the linter for the language and not disable them. An alternative could be to have a dropdown picker on the StatusBar, next to the warnings. Just as an icon or maybe showing there which linter is active, or none if there isn't one active and show in the dropdown only the linters available for the current language (if there are some).

ingorichter · 2013-08-01T18:33:16Z

After getting JSHint to work with the proposes API changes, I figured out that it would be good to have more than one linter/checker/analyzer per language. I made the changes locally to have JSLint and JSHint run for the same file. This is not a big change, but it requires a change to the results panel to distinguish between results provided from different linter/checker/analyzer.

TomMalbran · 2013-08-01T18:42:53Z

@ingorichter I thought about having multiple linters run on a file. Once tabs are implemented on the bottom panels, each linter's results could be added to its own tab, when there are errors. The status bar warning icon tooltip can then show the status of both linters. If JSLint gives warnings and JSHint doesn't, the icon would still be a warning, but the tooltip would say something like "n JSLint errors - No JSHint errors".

ingorichter · 2013-08-01T18:44:19Z

Having tabs is nice. In the meantime we could easily add another column to the results panel.

…nting * origin/master: (82 commits) add parens for optional union param Fix inconsistency in how pluralizable words are shown in the status bar - never use the indefinite "(s)" suffix anymore. prevent Theseus instrumentation of RemoteFunctions.js Code review: explicitly store JSLint state rather than using DOM; small cleanups to StatusBar code & docs (currently only used by JSLint). Removed hover text color. Redrew down-arrow icon. Down-arrow doesn't need hover state. Updated by ALF automation. Used waitsForDone function in test instead of waitsFor. Fixed JSLint errors in test file and addressed comments in pull request. Fixed usage of single quotes in code. Fixed previous merge. Fixed a case where there are more than 3 folders to display. Fixed JSLint errors and code formatting. Another go on algorithm for finding unique directory path Updated by ALF automation. Revert some recently added unneded JSLint fixes Update JSLint submodule to the commit we were using before switching to a submodule fix copyright year in finnish translation Rewritten algorithm for finding unique directory path ... Conflicts: src/extensions/default/JSLint/jslint.css src/extensions/default/JSLint/main.js

peterflynn · 2013-08-06T23:23:07Z

@TomMalbran: Exactly, the linter status icon could be interactive even when no linting results are currently shown. You could either add dropdowns there as you suggested, or simply pop open the panel and use the same in-panel enable/disable UI as you'd see when linting warning are present.

@ingorichter: Yes, definitely makes sense to allow multiple linters per language eventually. Luckily, I don't think that needs to change the API here at all! Tabs vs. a consolidated list is probably something to discuss in UI review when we get closer to actually doing that.

peterflynn · 2013-08-06T23:46:43Z

So my current thinking on naming is this:

Under the hood, refer to the API that linters register with as "CodeInspection" -- because not all code inspections are linters, but I think we can assume this API will only be used for inspections (not for manually-invoked build tools).
Under the hood, refer to the UI panel as "problems panel" (e.g. in CSS) -- because in the future this same piece of UI might show results from manually-invoked build tools. (In the future we could factor the panel UI part out into its own ProblemsPanel module, too -- without breaking the public CodeInspection API).
In the UI, continue using the terminology "linting" for now -- because it's more familiar to web devs than "inspections," and we can switch to the more generic "problems" later.

@dangoor Let me know if that sounds ok and I'll make the changes. I assume we won't get this landed today (before string freeze), so this can't land until Sprint 30. Thus I won't rush to do all the renaming today :-)

…king from the get-go)

dangoor · 2013-08-07T13:38:19Z

@peterflynn sorry I'm behind on this. I like your naming suggestions in general and agree that "inspections" is a better general term. The Chrome dev tools uses "Audits", but I've never personally been fond of that one. Will the "problems panel" ever be used for things that are not problems?

dangoor · 2013-08-16T14:47:57Z

@peterflynn One more comment about naming. We appear to have two idioms for naming the actual function to add a new thing:

QuickOpen.addQuickOpenProvider
CommandManager.register

this one uses a third:

Linting.registerLinter

I have a bit of a preference toward CommandManager.register. What do you think of something like CodeInspectors.register?

ingorichter · 2013-08-19T06:47:14Z

@dangoor I like CodeInspectors.register

ingorichter · 2013-08-23T01:19:35Z

I think it would be great, if we can make the suggested changes and merge them back to master. I'd like to finish the JSHint Extension based on this new API... ;-)

zaggino · 2013-08-23T01:31:43Z

@peterflynn I can't find something similar in the code, so will there be a method like

var results = Linting.checkFile( FileEntry );

so my extension will be able to lint a specific file?
I'm currently writing a Git extension that can commit files from Brackets UI and it'd be great to be able to show warnings that files being committed have some errors.

…nting * origin/master: (163 commits) Fixes after review Updated by ALF automation. Add ellipses to Replace All button so it's not as scary to click. (Without ellipses it looks like it will replace every occurrence immediately, which is how most "All" buttons in Find/Replace dialogs work). Show existing named flows from CSS hinter. Remove the buttons class from the bottom div and inherit buttons in the definition of bottom-buttons Updated by ALF automation. 2 small updates fix bug number reference reduce editor initial layout time by first removing doc selection Update find & replace result phrases Spanish strings updates verify parens are balanced before executing gradient regex Removed top and bototm transition for codehints. Add unit test Fix unit test by moving check for Active Line into _setEditorOption() Updated by ALF automation. fix live dev server prototype constructors Code hints shouldn't swallow Tab key. Don't commit code hints when tab key is pressed. begin refactoring of request filters to ServerRequestManager ... Conflicts: src/nls/root/strings.js

Rename linting UI panel to more generic "problems panel" But UI strings continue to use "linting" for now

TomMalbran · 2013-08-27T23:57:01Z

src/command/Commands.js

@@ -88,6 +88,7 @@ define(function (require, exports, module) {
    exports.VIEW_RESTORE_FONT_SIZE      = "view.restoreFontSize";
    exports.VIEW_SCROLL_LINE_UP         = "view.scrollLineUp";
    exports.VIEW_SCROLL_LINE_DOWN       = "view.scrollLineDown";
+    exports.VIEW_TOGGLE_LINTING         = "view.toggleLinting";


Should this be Toggle Inspection after the rename?

Yeah, that makes sense to me

zaggino · 2013-08-27T23:58:20Z

@peterflynn That'd be great. If you could write it down to a trello card or somewhere where I can "watch" for it, I'd be grateful ;-)

TomMalbran · 2013-08-28T00:00:19Z

src/nls/root/strings.js

@@ -454,15 +454,20 @@ define({
    "CMD_JUMPTO_DEFINITION"                     : "Jump to Definition",
    "CMD_SHOW_PARAMETER_HINT"                   : "Show Parameter Hint",
    "NO_ARGUMENTS"                              : "<no parameters>",
-
+
+    // CodeInspection: errors/warnings


All this strings should be moved after the Statusbar strings.

Done. I rearranged a few other strings as well so the order is more consistent.

TomMalbran · 2013-08-28T00:01:32Z

@zaggino You could also make a PR after this one is merged ;)

It looks like by just making run receive a Document and a new public API that calls run with the received Document would solve what you want.

zaggino · 2013-08-28T00:17:18Z

@TomMalbran good point 👍, haven't thought of that really, probably too much in my head right now 😕

dangoor · 2013-09-06T13:13:33Z

src/language/CodeInspection.js

@@ -0,0 +1,373 @@
+/*
+ * Copyright (c) 2012 Adobe Systems Incorporated. All rights reserved.


Nit: it's 2013 😃

MiguelCastillo · 2013-09-06T13:56:38Z

This is really cool and interesting. I have just made my interactive-linter extension plugin friendly because I needed an easy way to drop in new linters... The work was primarily done so that I can add support for coffeelint, which I am hoping people will find useful.

I may wire in my stuff so that is also reports into this new system you guys are putting in place. That would be really nice!

dangoor · 2013-09-06T14:06:04Z

Hey, @peterflynn

Review complete. The items mentioned were all very minor and many are things people could reasonably disagree about. Feel free to merge after making those minor changes (or not).

If the manual merge requires bigger changes or you spot something that needs more changes, feel free to ask for review of the changes.

- Rename more vars to avoid "lint" terminology - Always append "+" when scan aborted, not just when > 1 error found so far - Clean up toggle-enablement API

@TomMalbran

…nting * origin/master: (275 commits) Change copyright to 2013 Change copyright to 2013 Comment tweaks. Renamed addPos/offsetPos to _addPos/_offsetPos since they're private. Fix test for new status with LiveHTML Delete strings-app.js and remove "Gettings started line" Get some recent fixes from upstream Updated by ALF automation. Add comments for `generateAttributeEdits` Flesh out the comments for SimpleDOM. Replace .children checks with .isElement() Remove a bunch of unused variables (from review #5065) Fixed quiet scrollbar based on @TomMalbran's feedback. Removed base64 PNGs as they're no longer required. Updated year. Fixed based on @TomMalbran's feedback. Updated by ALF automation. Removed linux scrollbar style. Cleaned up a bit. Updated link. Removed quiet scrollbar styles as they're all in brackets_scrollbars.less now. ... Conflicts: src/brackets.js

peterflynn · 2013-09-06T20:27:01Z

@dangoor Changes pushed -- do you want to give it a quick skim before it gets merged?

peterflynn · 2013-09-06T20:29:21Z

@zaggino It sounds like the API you want is more discrete than just invoking run(), no? Probably just hoisting out the logic from run() that finds the right provider and calls it -- skipping all the stuff about updating the results panel UI. That's probably not a high priority for us on core, but it seems simple enough to do and we'd happily accept a pull request! :-)

peterflynn · 2013-09-06T20:32:45Z

@MiguelCastillo Long term I would definitely like to integrate real-time linting into Brackets core (which I think we could do transparently, without breaking the API here). But in the meantime, if you wanted to leverage the pluggability created here, we could add an API that just fetches the active linting provider for a given document -- which your own real-time linter could then invoke as it sees fit. (Come to think of it, that would address what @zaggino wanted too!)

ingorichter · 2013-09-06T22:30:43Z

@peterflynn are there any code changes that you wanted to make? Or can I merge your changes?

peterflynn · 2013-09-06T23:58:47Z

Ok, I'll go ahead and merge now considering @dangoor's comment from earlier today.

Pluggable linting

zaggino · 2013-09-07T00:10:31Z

@peterflynn nice, I'll try to put up a PR soon :)

ingorichter · 2013-09-07T00:22:29Z

Thanks @peterflynn. I'll polish the jshint extension and prepare it for submission.

dangoor · 2013-09-07T02:38:59Z

The changes look good. Thanks for the cleanup and merging, @peterflynn

peterflynn added 4 commits July 25, 2013 18:24

Initial separation of JSLint extension into generic linting manager (in

1beb97d

core) and headless JSLint-specific 'linting provider' (in extension)

More linting extensibility work:

4f457f9

* Separate out "JSLint" strings, so linter name is also extensible * Nicer-structured linting results API; JSLint extension translates JSLint results into this generic format * Different tooltips for linting disabled vs. no linter for filetype

* Add a META linting level so core linting code doesn't have assumptions

1776519

about decrementing the error total. * Remove "JSLint" from toggle-enabled command * Fix bug where tooltip string for the disabled case wasn't used

ghost assigned njx and dangoor Jul 30, 2013

Fix bug spotted by @ingorichter in #4662 (seems worth having this wor…

7b53bfb

…king from the get-go)

peterflynn added 2 commits August 27, 2013 01:04

Rename linting APIs to more generic "CodeInspection"

fe86632

Rename linting UI panel to more generic "problems panel" But UI strings continue to use "linting" for now

TomMalbran reviewed Aug 27, 2013
View reviewed changes

TomMalbran reviewed Aug 28, 2013
View reviewed changes

dangoor reviewed Sep 6, 2013
View reviewed changes

peterflynn added 2 commits September 6, 2013 12:55

Code review:

820b4c6

- Rename more vars to avoid "lint" terminology - Always append "+" when scan aborted, not just when > 1 error found so far - Clean up toggle-enablement API

peterflynn added a commit that referenced this pull request Sep 7, 2013

Merge pull request #4588 from adobe/pflynn/pluggable-linting

db4e426

Pluggable linting

peterflynn merged commit db4e426 into master Sep 7, 2013

peterflynn deleted the pflynn/pluggable-linting branch September 7, 2013 00:01

zaggino mentioned this pull request Sep 8, 2013

Expose code inspection providers for the extensions. #5125

Merged

This was referenced Aug 29, 2021

[CLOSED] Pluggable linting brackets-archive/bracketsIssues#4244

Open

[CLOSED] Expose code inspection providers for the extensions. brackets-archive/bracketsIssues#4714

Open

		@@ -0,0 +1,373 @@
		/*
		* Copyright (c) 2012 Adobe Systems Incorporated. All rights reserved.

Pluggable linting #4588

Pluggable linting #4588

Conversation

peterflynn commented Jul 27, 2013

peterflynn commented Jul 27, 2013

pthiess commented Jul 27, 2013

TomMalbran commented Jul 27, 2013

ingorichter commented Jul 29, 2013

njx commented Jul 29, 2013

peterflynn commented Jul 30, 2013

peterflynn commented Jul 30, 2013

TomMalbran commented Jul 30, 2013

peterflynn commented Jul 30, 2013

TomMalbran commented Jul 30, 2013

ingorichter commented Aug 1, 2013

TomMalbran commented Aug 1, 2013

ingorichter commented Aug 1, 2013

peterflynn commented Aug 6, 2013

peterflynn commented Aug 6, 2013

dangoor commented Aug 7, 2013

dangoor commented Aug 16, 2013

ingorichter commented Aug 19, 2013

ingorichter commented Aug 23, 2013

zaggino commented Aug 23, 2013

TomMalbran Aug 27, 2013

Choose a reason for hiding this comment

dangoor Sep 6, 2013

Choose a reason for hiding this comment

zaggino commented Aug 27, 2013

TomMalbran Aug 28, 2013

Choose a reason for hiding this comment

peterflynn Sep 6, 2013

Choose a reason for hiding this comment

TomMalbran commented Aug 28, 2013

zaggino commented Aug 28, 2013

dangoor Sep 6, 2013

Choose a reason for hiding this comment

MiguelCastillo commented Sep 6, 2013

dangoor commented Sep 6, 2013

peterflynn commented Sep 6, 2013

peterflynn commented Sep 6, 2013

peterflynn commented Sep 6, 2013

ingorichter commented Sep 6, 2013

peterflynn commented Sep 6, 2013

zaggino commented Sep 7, 2013

ingorichter commented Sep 7, 2013

dangoor commented Sep 7, 2013