You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
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?
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?
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.
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.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #8658I left some directive like
regexp, forin
or some globals likeappshell, less
inline because seemed better for now.