[CLOSED] Move most inline jslint directives to config files

[core-ai-bot]

Issue by ficristo
Tuesday Aug 09, 2016 at 18:06 GMT
Originally opened as adobe/brackets#12661

---

Move most of the inline jslint directives inside .brackets.json and .eslintrc.json.
I've removed the browser directive too and so this PR supersedes #8658
I left some directive like regexp, forin or some globals like appshell, less inline because seemed better for now.

---

ficristo included the following code: https://github.com/adobe/brackets/pull/12661/commits

[core-ai-bot]

Comment by ficristo
Wednesday Aug 10, 2016 at 13:33 GMT

---

I self reviewed a bit this and I saw an issue...
I'll look at this more carefully and with more calm.

[core-ai-bot]

Comment by MarcelGerber
Thursday Aug 11, 2016 at 22:11 GMT

---

Some things I've noticed during reviewing the 6500-lines diff (I probably shouldn't do this at 11pm...), in no particular order:

• Should we mark XMLHttpRequest a global as well? Scarcity doesn't seem to be an argument here because other seldomly used objects like Uint32Array are also marked as global.
• What's it about document vs window.document. I know they behave the same, but the latter seems kind of counterintuitive to me. And when coding new... things... one's probably still inclined to use document for the sake of brevity, just to get an JSLint error. Should we mark it defined as well?
• Have you checked whether the predefs in test files (beforeEach, afterEach, spyOn, runs) are actually used in that specific file? Seems like we might be able to get some of them off the list, but it's up to you whether you think it's worth doing.
• src/LiveDevelopment/launch.html still has a /*jslint comment, although not at the top. I don't think it's needed for live preview, so you can probably remove it as well.
• Same goes for Prefs.js and main.js in src/extensions/default/CodeFolding, and src/extensions/default/UrlCodeHints/main.js, and all files in src/extensions/default/CodeFolding/foldhelpers
• I don't really know where event in src/project/FileViewController.js comes from, but it's probably not a global (window.event)
• nit: Sometimes you have it /*jslint something: true */ (with space), sometimes without
• In the case of ExtensionManager-test, the globals are like /*global describe: false, .... Maybe we should unify these some more.

Also, I'm curious to hear which issue you found?

[core-ai-bot]

Comment by ficristo
Friday Aug 12, 2016 at 09:20 GMT

---

Thank you for review.

• added XMLHttpRequest to the global list
• regarding document see [CLOSED] ALF Automation #8658 for details: Even document is an issue since we often use local vars named document to reference Document objects
• I didn't check if the globals are used in test files. It's possible, if we add tests, we will need to readd them so I left the test files as are for now
• I didn't check html files. I'm not even sure jslint works there. For now I left as is.
• I skipped the CodeFolding extension waiting for a PR to land. Fixed now.
• for FileViewController you can look at [CLOSED] test #12145: This "event" handle is from the global context. If the the current execution frame is a result of an event triggered by user or system , this handle is always set at the global context. As the actual event handle is never passed through the call stack , I have used the global handle to verify the context.
• made the spacing consistent
• I've removed the false attribute in the global case

For the issue I found, I changed a document variable to window.document in src/LiveDevelopment/MultiBrowserImpl/protocol/remote/DocumentObserver.js, but that variable was actually a function parameter. Now I fixed it.

If we remove the inline jslint directives I suppose we loose the ability to lint our code with other tools.
For me this PR is still an improvement, but I'm not sure if we want the ability to lint with other tools.
Maybe we can rely to only ESLint for that.
Thoughts?

[core-ai-bot]

Comment by MarcelGerber
Friday Aug 12, 2016 at 19:28 GMT

---

There may be some folks who want to run JSHint on the Brackets source, but the official tools are definitely JSLint, and, even more so, ESLint.
So to me it's fine, but I'd like to hear at least one other opinion before I'm willing to merge this PR (which LGTM now).
Maybe@petetnt@abose@zaggino have something to say here?

[core-ai-bot]

Comment by zaggino
Friday Aug 12, 2016 at 23:13 GMT

---

LGTM from me. I'm of the opinion that we should completely ditch JSLint (and JSHint) in favor of ESLint making them just optional extensions downloadable from extension registry for those, who want them. Prerequisite for that would be of course shipping ESLint support in the core.

[core-ai-bot]

Comment by petetnt
Saturday Aug 13, 2016 at 10:15 GMT

---

I am all in favor of using ESLint exclusively both in the core and as the default extension. IMHO it's the current defacto tool and has the most adjustable rules and features.

One idea would be also spinning the config off to its own module so it could be used for extension development too.

[core-ai-bot]

Comment by MarcelGerber
Saturday Aug 13, 2016 at 17:17 GMT

---

LGTM. Thank you!