Code inspection improvements #5935

ingorichter · 2013-11-11T17:45:24Z

Adding support (functional and partly visual) for having multiple code linters for the same document type.
This means you can have both JSLint and JSHint (it's questionable if this makes sense) linter produce linting results.

Once a linter was registered for a file type, it will run with every save operation (unless this is disabled in the UI). There is no exposed UI way to enable/disable a linter, this means at the moment "All or nothing". You can uninstall the extension that provides the linter functionality to get rid of it's warnings. It's still TBD how a UI for this could look like.

Here is how ti looks like for a JSHint and JSLint fighting for your attention:

- added a new string for the Code Linting Result Panel Title

- added more tests

Conflicts: src/language/CodeInspection.js

peterflynn · 2013-11-12T01:26:44Z

src/nls/root/strings.js

@@ -186,6 +186,7 @@ define({
    "STATUSBAR_LINE_COUNT_PLURAL"           : "\u2014 {0} Lines",

    // CodeInspection: errors/warnings
+    "CODE_INSPECTION_PANEL_TITLE"           : "Code Inspection",


I won't be able to do a full code review until tomorrow, but because string freeze is today we need to at least finalize the strings tonight (we can merge them into master alone in a separate PR so they land in time).

I'd be hesitant to change the panel title to "Code Inspection" when everything else in the UI uses the term "lint" still. Here are some alternatives:

Generic string like "Problems" or "Lint Results"

String with total count (similar to Find in Files panel header), like "14 Problems" or "14 Lint Problems"

Show name of provider (same as today) if only one registered; fall back to one of the above string options if > 1 provider

I'm leaning toward the last option, with the fallback being a numeric one like "14 Lint Problems" or "14 Lint Issues." @ingorichter @njx thoughts?

I wound up going with the last suggestion and merging that in before string freeze. So this PR needs to be updated to use the new strings.

It looks like this doesn't reflect the strings that were merged in last sprint -- see discussion above and #5953.

Sorry, I've missed this part...

…int 34 ends (but after string freeze has begun)

Strings for use with PR #5935

peterflynn · 2013-11-13T19:29:12Z

src/language/CodeInspection.js

@@ -38,7 +38,7 @@
 */
 define(function (require, exports, module) {
    "use strict";
-    
+


Nit: there are a lot of whitespace diffs in this file -- probably from some sort of 'whitespace cleaner' extension. It's best to disable such extensions when editing the Brackets source code (or any code that's typically edited in Brackets), since Brackets will always generate indented whitespace like this... creating a never-ending stream of diffs every time you commit. Almost half the diffs here are just whitespace.

Argh, I forgot about the whitespace removal. My apologies. I like to have everything clean and tidy. Old habits. Perhaps you could apply ?w=1 to the url and don't get bothered by too many whitespace changes.

peterflynn · 2013-12-09T03:06:32Z

src/language/CodeInspection.js

@@ -187,82 +201,151 @@ define(function (require, exports, module) {
        return response.promise();
    }

+    function updatePanelTitleAndStatusBar(numProblems, providerReportingProblems, providerList, aborted) {
+        // don't show a header if there is only one provider available for this file type


Seems like this comment belongs with the line that calls hide()

Yes. Moved to it's original location.

peterflynn · 2013-12-09T03:42:05Z

@ingorichter Done rereviewing -- getting close!

Also looks like there's a merge conflict somewhere that needs to get resolved...

peterflynn · 2013-12-09T08:23:32Z

src/language/CodeInspection.js


                PerfUtils.addMeasurement(perfTimerInspector);
-                response.resolve(result);


This is a bit subtle, but: putting addMeasurement() after response.resolve() -- instead of before like it is on master -- means the measured time will include everything in the caller's done() handler as well, which in this case includes all the table-generating code in run(). Could you switch the order back so this matches master?

changed the order to match the one on master

peterflynn · 2013-12-14T04:25:36Z

@ingorichter You think you still have time to do those last fixes? Otherwise we may want to punt this to Sprint 36...

Conflicts: src/nls/root/strings.js

ingorichter · 2013-12-20T01:36:12Z

@peterflynn If you are not too tired of reviewing this... I hope, that I didn't miss anything this time

busykai · 2013-12-30T19:53:56Z

src/language/CodeInspection.js

+                html = Mustache.render(ResultsTemplate, {reportList: allErrors});
+            });
+
+            // Update results table


This entire block, lines 332-350 up to else, should go into .then handler where html is prepared. Otherwise it will be operating empty html since the template is rendered in .then handler (see line 329 above). It will show up once there's a real delay in provider execution.

Yep, and that's how it is on master before this PR...

peterflynn · 2014-01-09T02:09:42Z

src/language/CodeInspection.js

+            // This is a special case to enable extension provider to replace the JSLint provider
+            // in favor of their own implementation
+            _.remove(_providers[languageId], function (registeredProvider) {
+                return registeredProvider.name === "JSLint";


This is a little delicate since it relies on the localized name field. But I've checked and all locales use the string "JSLint," and this code will go away once there's UI for enabling/disabling providers -- so I think this is ok for now.

Yes, I didn't find something better here. But moving forward, this code should be removed sooner or later.

…ction-improvements * origin/master: (187 commits) improve comments update comments Fix a comment typo and just log the unhandled exception instead of trying to reject the promise, which probably won't work anyway. Enable Quick Docs for LESS Return a promise from ProjectManager.getAllFiles that is resiliant to misbehaving done handlers Updated by ALF automation. Revert LiveDev timeout changes Use Brackets shell for copying files to reduce the number of external watcher notifications and increase timeouts Don't refresh the file tree on change events for directories outside of the project root, but when we do refresh the file tree, also clear the change queue (using a new PromiseQueue.removeAll method) and use a debounced refreshFileTree call fix check for added and removed files in fs change event handler always hide dirty dot if there is no doc Fix selection update after fs change event. Remove unnecessary updates when no added or removed files are present. backout change for dismissing list Right aligning numbers column. Removed most inline css. Adding LOCALE_KO Initial commit. Fixed some dutch spelling/translation errors. Bump up timeouts to account for additional Project Manager synchronization Made canvas height 300 to stop weird rounding. ... Conflicts: src/language/CodeInspection.js

peterflynn · 2014-01-09T03:46:35Z

@ingorichter I took the liberty of pushing up a commit that fixes the last few remaining comments I had (including the issue @busykai was hitting in #6375), along with a few more documentation updates.

So from my standpoint, I think it's good to land now. Do you want to take a look at my commit before merging?

peterflynn · 2014-01-09T04:46:48Z

I've also filed bugs on the two extensions that will be broken by the inspectFile() API change, and documented the change in the release notes.

ingorichter · 2014-01-09T19:06:56Z

@peterflynn Thank you for the last fixes. Tests passed. I think, we can go ahead and merge it. 👍

peterflynn · 2014-01-09T19:33:55Z

Oops, I realized the docs for inspectFile() said something incorrect (and had some mis-formatted type annotations) -- just pushed up a fix for that too. I think those docs-only fixes are noncontroversial enough that I can just go ahead & merge without further review.

…ents Code inspection improvements - allow multiple linters / code inspectors to be registered for each Language, with consolidated results

ingorichter added 5 commits November 5, 2013 18:21

- added support for multiple code linter per file type

88ea455

- added some unit tests for Code Linting

0b6fb39

- added a new string for the Code Linting Result Panel Title

- removed code to enable/disable code linter

e5f497c

- added more tests

Merge branch 'master' into irichter/code-inspection-improvements

9e92cf0

Conflicts: src/language/CodeInspection.js

- update the comments

eb6b7bc

ingorichter mentioned this pull request Nov 11, 2013

- modified the problems panel to support multiple code inspector #5235

Closed

peterflynn reviewed Nov 12, 2013
View reviewed changes

peterflynn added a commit that referenced this pull request Nov 12, 2013

Strings for use with PR #5935 in case we can get it landed before Spr…

74dafa4

…int 34 ends (but after string freeze has begun)

peterflynn mentioned this pull request Nov 12, 2013

Strings for use with PR #5935 #5953

Merged

peterflynn added a commit that referenced this pull request Nov 12, 2013

Merge pull request #5953 from adobe/pflynn/strings-for-multilint

38de6e0

Strings for use with PR #5935

ghost assigned peterflynn Nov 12, 2013

peterflynn reviewed Nov 13, 2013
View reviewed changes

peterflynn reviewed Dec 9, 2013
View reviewed changes

ingorichter added 2 commits December 19, 2013 17:30

- some changes based on review comments

4160ab4

Merge branch 'master' into irichter/code-inspection-improvements

20e5d5f

Conflicts: src/nls/root/strings.js

busykai reviewed Dec 30, 2013
View reviewed changes

busykai mentioned this pull request Jan 6, 2014

[REVIEW ONLY] Bare async provider implementation. #6375

Closed

peterflynn reviewed Jan 9, 2014
View reviewed changes

peterflynn added 2 commits January 8, 2014 19:37

Address remaining code review comments and update/fix documentation

12d33b9

More documentation corrections

e4f2153

peterflynn merged commit ad5f5d7 into master Jan 9, 2014

peterflynn deleted the irichter/code-inspection-improvements branch January 9, 2014 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code inspection improvements #5935

Code inspection improvements #5935

ingorichter commented Nov 11, 2013

peterflynn Nov 12, 2013

peterflynn Nov 13, 2013

peterflynn Dec 3, 2013

ingorichter Dec 4, 2013

peterflynn Nov 13, 2013

ingorichter Nov 13, 2013

peterflynn Dec 9, 2013

ingorichter Dec 9, 2013

peterflynn commented Dec 9, 2013

peterflynn Dec 9, 2013

ingorichter Dec 9, 2013

peterflynn commented Dec 14, 2013

ingorichter commented Dec 20, 2013

busykai Dec 30, 2013

peterflynn Jan 9, 2014

peterflynn Jan 9, 2014

ingorichter Jan 9, 2014

peterflynn commented Jan 9, 2014

peterflynn commented Jan 9, 2014

ingorichter commented Jan 9, 2014

peterflynn commented Jan 9, 2014


		PerfUtils.addMeasurement(perfTimerInspector);
		response.resolve(result);

Code inspection improvements #5935

Code inspection improvements #5935

Conversation

ingorichter commented Nov 11, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Dec 9, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Dec 14, 2013

ingorichter commented Dec 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Jan 9, 2014

peterflynn commented Jan 9, 2014

ingorichter commented Jan 9, 2014

peterflynn commented Jan 9, 2014