Expose code inspection providers for the extensions. #5125

zaggino · 2013-09-08T04:20:06Z

As mentioned in #4588 (comment)

API that fetches the active linting provider for a given file entry.

zaggino · 2013-09-08T04:34:18Z

@peterflynn can you review? i choose rather not to modify the original run function

zaggino · 2013-09-13T13:20:08Z

@peterflynn could we do this for sprint 31 please? :)

peterflynn · 2013-09-13T15:02:06Z

src/language/CodeInspection.js

+        var language   = LanguageManager.getLanguageForPath(fileEntry.fullPath),
+            languageId = language && language.getId(),
+            provider   = language && _providers[languageId];
+        return provider;


This duplicates code from run(), so I think you should modify run() to use this. I don't think it will be too disruptive to hoist out.

true, but you used language and languageId later, so you mean like return all three variables in object ?

peterflynn · 2013-09-13T15:12:42Z

What would you think of changing the API to something like: inspectFile(fullPath), returning the provider result? This way the provider running code could evolve without clients of this API needing to replicate all those changes. This would also let CodeInspection take care of loading the file from disk (or reusing a Document if open already) instead of each client of the API having to do that.

One thing I have in mind is the possibility of providers becoming async (#5137). An inspectFile() type API would be async right out of the box, so clients wouldn't break if async providers come along later.

peterflynn · 2013-09-13T15:13:02Z

@zaggino Done reviewing. Just those two thoughts.

zaggino · 2013-09-13T16:26:49Z

@peterflynn Take a look please what do you think. Haven't modified any of the original code yet but this inspectFile() implementation would work for me quite nicely.

peterflynn · 2013-09-13T16:36:56Z

src/language/CodeInspection.js

+            .done(function (fileText) {
+                var perfTimerInspector = PerfUtils.markStart("CodeInspection '" + languageId + "':\t" + fileEntry.fullPath),
+                    scanPromise = $.when(provider.scanFile(fileText, fileEntry.fullPath));
+                Async.withTimeout(scanPromise, SCAN_TIMEOUT)


I don't think we need to wrap this in a timeout -- we generally don't try to protect against bad/slow async extension code anywhere else

Ok, I'll remove it then. Will finish this tomorrow, almost 3am already in Syd :)

peterflynn · 2013-09-13T16:47:04Z

@zaggino This is looking like a good direction, yeah.

zaggino · 2013-09-14T03:58:38Z

Done, with a bit of optimization. Original code was multiplying event handlers on the results table with every render. That should be solved now. Please review @peterflynn

peterflynn · 2013-09-16T23:36:28Z

@zaggino I haven't been able to give the latest changes a close look yet -- hopefully tomorrow.

But from my quick glance, I'm a little concerned about possible race conditions in run(), e.g. if you switch files before the provider has returned results. Since we're getting close to the end of the sprint and thus a bit more risk-averse, I wonder if the best thing for now would be to take out the support for async providers. That makes it easier to land the API you need before the sprint ends -- and we could introduce async providers next sprint without breaking API compatibility. What do you think?

zaggino · 2013-09-16T23:39:24Z

@peterflynn it's not very difficult to check if the checked document is still the current document when the async provider finishes it's work... but I can disable it in this one to be safe and put up another PR with async

zaggino · 2013-09-16T23:41:09Z

and right now - unless someone installs an extension with async provider, it won't be used anyway...

peterflynn · 2013-09-17T08:55:41Z

@zaggino There are other cases too, though -- you might modify & save the file a second time while the first linting is still running, or toggle the panel visibility, or disable linting... We probably shouldn't offer API support until we're sure it will work reliably.

zaggino · 2013-09-17T09:41:32Z

ok @peterflynn , i'll remove the async parts to keep this simple for now

zaggino · 2013-09-17T09:51:17Z

Done, inspectFile is still async for cases when the fileEntry is not found in the DocumentManager.getOpenDocumentForPath. Do you think that this is also a problem?

zaggino · 2013-09-23T04:29:26Z

@peterflynn Can we merge now? I see Sprint 32 in the master.

peterflynn · 2013-09-25T00:28:17Z

src/language/CodeInspection.js

+
+    /**
+     *  Gets a provider for given fileEntry, if one is available
+     */


Needs type annotations for argument & return values. Ditto for inspectFile() below -- every public API (at minimum) should have these docs.

Also, nit: the double spacing after the * is inconsistent with how the rest of the file is formatted.

peterflynn · 2013-09-25T00:48:47Z

@zaggino Looking good -- just a few minor comments.

peterflynn · 2013-09-25T01:01:01Z

Actually two other thoughts:

Should we pass in FileEntry objects, or just string fullPaths? Although I sorta like the semi-type-safetiness of using FileEntry, I think most of our APIs right now just take paths. @gruehle any opinion on which is more future-proof? ;-)
getProvider() should be suffixed, e.g. getProviderForPath() or ...ForFile() depending on which we choose above.

zaggino · 2013-09-25T01:18:04Z

Fixed the comments. About the FileEntry vs FullPath thing, currently in my extension I have this piece of code to call the method, so fullPath would be easier in my situation but might not be better for others. return CodeInspection.inspectFile(new NativeFileSystem.FileEntry(Main.getProjectRoot() + filename));

peterflynn · 2013-09-25T01:32:34Z

Oops, actually one more thing, now that I've been running on this branch: the built-in JSLint provider returns null anytime there are no errors, so the console winds up littered with warnings. I wonder if we should actually get rid of the warning and just continue to allow null as a return value? Now that run() uses getProvider() as a helper, we don't really need non-null results to help disambiguate no-provider vs. no-errors...

zaggino · 2013-09-25T02:00:09Z

Ok, I've removed the warning.

peterflynn · 2013-09-25T05:56:37Z

Great! I'll wait until tomorrow morning to see if @gruehle has any thoughts on the FileEntry vs. fullPath question, but personally I'm leaning toward the path. It's easy to call that form whether you have a FileEntry wrapper or not, which is nice. (And then we should rename the API accordingly).

Also had one other suggestion, if we're going to make one more commit anyway: I think the docs for inspectFile() should mention that it will reflect any unsaved changes present in the file, if it's currently open. Might also be worth clarifying that it just computes the results headlessly & doesn't update the UI -- or at least changing the phrasing to avoid using the word "Runs" which could be confused with the runs() function.

zaggino · 2013-09-25T06:28:25Z

Ok, will fix that comments ... also I can squash these commits into one before merging, so the history of the main repo is not polluted unnecessarily with too many commits.

gruehle · 2013-09-25T14:34:51Z

Yeah, I prefer using a pathname for getProvider(), but sticking with a FileEntry for inspectFile().

I had another thought: why not store the code inspection provider on the language object? This way you can just use getLanguageForPath().inspectionProvider. We already store the providers by the language ID anyway...

zaggino · 2013-09-25T23:49:15Z

Did the getProvider() change, can also do the language object thing if desired.

One more thought, currently there is only one possible file inspector for one language but in the future, there might be desired feature to support multiple code inspectors who check for different things. Like I can use jshint on my JavaScript files but I also want an extension that checks for patterns that are forbidden in a specific framework like dojo or yui.

peterflynn · 2013-09-26T08:14:28Z

@zaggino Very good point. So, what's the use case for getProvider() at this point? The motivation for inspectFile() originally was to help insulate callers from dealing with the provider directly, so ideally if we've done out job getProvider() is unneeded. If we offered a simpler hasProvider() API instead (in combination with inspectFile()), would that be good enough?

peterflynn · 2013-09-26T08:18:23Z

@gruehle We have other things that are language-specific too, like code hint providers, Quick Open providers, etc. and we don't hang those directly off the Language object. We could stuff them all into the Language, but it starts to feel a little iicky to me in a separation-of-concerns sense... feels nicer to have each of those subsystems own its own data.

zaggino · 2013-09-26T10:58:05Z

I've removed getProvider from exports as I too don't see much use for it. Currently from outside you can call inspectFile with any provider you want or leave Brackets to use the default provider which seems fine to me.

Currently if there's no default provider for the given file type, inspectFile returns null. I'm not sure we need to publish hasProvider because this doesn't say much to anyone since from true / false one can't really know, what the provider does.

gruehle · 2013-09-26T19:27:55Z

@peterflynn Good point about the other language-specific features. I agree that it doesn't make sense to clutter up Language like that.

Making getProvider() private is an even better way the solve the problem 😄

peterflynn · 2013-09-26T21:58:48Z

@zaggino Looks good to go! Did you still want to make a squashed version?

First go at inspectFile method. Removed Async.withTimeout from inspectFile() Separated getProvider and inspectFile logic, added usage of DocumentManager.getOpenDocumentForPath to inspectFile Modified run() to use new methods. Moved $problemsPanelTable event handlers from run() to htmlReady() for optimization. Fix the case when getCurrentDocument returns null. Check if current document hasn't changed while inspectFile was running remove async handling of provider.scanFile Refactored according to comments. Made provider response nullable again, removed the warning. Refactored getProvider to getProviderForPath and updated docs. Remove getProvider from exports.

zaggino · 2013-09-26T22:37:10Z

Yes, that'll be better. Squashed now.

zaggino · 2013-09-27T02:29:28Z

@peterflynn ready to go, but this will certainly create merge problems with #5235

peterflynn · 2013-09-27T22:46:50Z

Looks good -- thanks for being patient! #5235 is just starting review, so there's plenty of time to sort out the conflicts over there. I think this one is ready to land now.

Expose API to run code inspection as a headless one-off on an arbitrary file

ghost assigned peterflynn Sep 9, 2013

peterflynn reviewed Sep 13, 2013
View reviewed changes

zaggino mentioned this pull request Sep 15, 2013

Brackets-Git stopped working in the latest Brackets master brackets-userland/brackets-git#7

Closed

peterflynn reviewed Sep 25, 2013
View reviewed changes

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

Merge pull request #5125 from zaggino/codeInspectionApi

c06e73d

Expose API to run code inspection as a headless one-off on an arbitrary file

peterflynn merged commit c06e73d into adobe:master Sep 27, 2013

zaggino deleted the codeInspectionApi branch September 29, 2013 02:35

core-ai-bot mentioned this pull request Aug 29, 2021

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

Open

Expose code inspection providers for the extensions. #5125

Expose code inspection providers for the extensions. #5125

Conversation

zaggino commented Sep 8, 2013

zaggino commented Sep 8, 2013

zaggino commented Sep 13, 2013

peterflynn Sep 13, 2013

Choose a reason for hiding this comment

zaggino Sep 13, 2013

Choose a reason for hiding this comment

peterflynn commented Sep 13, 2013

peterflynn commented Sep 13, 2013

zaggino commented Sep 13, 2013

peterflynn Sep 13, 2013

Choose a reason for hiding this comment

zaggino Sep 13, 2013

Choose a reason for hiding this comment

peterflynn commented Sep 13, 2013

zaggino commented Sep 14, 2013

peterflynn commented Sep 16, 2013

zaggino commented Sep 16, 2013

zaggino commented Sep 16, 2013

peterflynn commented Sep 17, 2013

zaggino commented Sep 17, 2013

zaggino commented Sep 17, 2013

zaggino commented Sep 23, 2013

peterflynn Sep 25, 2013

Choose a reason for hiding this comment

peterflynn Sep 25, 2013

Choose a reason for hiding this comment

peterflynn commented Sep 25, 2013

peterflynn commented Sep 25, 2013

zaggino commented Sep 25, 2013

peterflynn commented Sep 25, 2013

zaggino commented Sep 25, 2013

peterflynn commented Sep 25, 2013

zaggino commented Sep 25, 2013

gruehle commented Sep 25, 2013

zaggino commented Sep 25, 2013

peterflynn commented Sep 26, 2013

peterflynn commented Sep 26, 2013

zaggino commented Sep 26, 2013

gruehle commented Sep 26, 2013

peterflynn commented Sep 26, 2013

zaggino commented Sep 26, 2013

zaggino commented Sep 27, 2013

peterflynn commented Sep 27, 2013