Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Move most inline jslint directives to config files #12661

Merged
merged 1 commit into from
Aug 13, 2016

Conversation

ficristo
Copy link
Collaborator

@ficristo ficristo commented Aug 9, 2016

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
Copy link
Collaborator Author

ficristo commented Aug 10, 2016

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

@marcelgerber
Copy link
Contributor

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?

@ficristo
Copy link
Collaborator Author

Thank you for review.

  • added XMLHttpRequest to the global list
  • regarding document see Restore 'browser: false' to Brackets core's JSLint setup #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 Bug Fix for #12111 and #12144 #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?

@marcelgerber
Copy link
Contributor

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?

@zaggino
Copy link
Contributor

zaggino commented Aug 12, 2016

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.

@petetnt
Copy link
Collaborator

petetnt commented Aug 13, 2016

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.

@marcelgerber
Copy link
Contributor

LGTM. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants