Mozlint provides a common way to add linters to mozilla-central and a common way to run them. For example, the idea is to run something like: ./mach lint testing/mochitest and have both python and js files linted. Another use case would be something like: ./mach lint -r <rev> which would lint all files touched by <rev> no matter what file extension. My goal here is to maintain complete backwards compatibility with the existing 'eslint' mach command. The only differences should be how eslint gets invoked internally, and developers / CI should not be affected by these changes.

Review commit: https://reviewboard.mozilla.org/r/41425/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41425/

This commit simply moves 'testing/eslint' to 'tools/lint/eslint' and the eslint related mach command from 'python/mach_commands.py' to 'tools/lint/mach_commands.py'. It shouldn't have any functional change on running eslint, either through mach or taskcluster. This is in preparation for bug 1258341, to make the diffs there a little easier to read. Review commit: https://reviewboard.mozilla.org/r/57298/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57298/

Comment on attachment 8784956 [details] Bug 1258341 - Modify 'eslint' implementation to work with the mozlint framework, Hey Mike, Ryan, Patrick! I'm flagging you for feedback because this is a pretty major change to the eslint infrastructure. If you don't care as long as it works, feel free to unflag yourself and ignore this. I just wanted to make sure I'm not surprising anyone. The intent is that |mach eslint| should work almost exactly the same as before (with a couple caveats mentioned in the commit message). To re-iterate, this just changes how the 'eslint' binary gets invoked by python. None of the npm stuff or configuration should be changing as a result of this. In case you're wondering, here is what we gain by using mozlint: * Can run |mach lint -r <rev>| or |mach lint -w| to lint only files touched by commit(s) or working directory (hg and git) * Can see errors from multiple linters interleaved in the same output. For example, if your patch touches both JS and python files, it will run both eslint and flake8 and show the output of both in the same format. The immediate benefit of this is small, but should increase as more linters get added in the future. * Provides a single integration layer for linting. For example, smacleod is going to be adding mozlint to mozreview. This way he'll be able to do the integration once, and get all the supported for linters for free (rather than needing to special case eslint). Similar integrations might be made for e.g editors in the future. Anyway, please let me know if you see something suspect or are concerned by any of these changes. Thanks!

Comment on attachment 8784956 [details] Bug 1258341 - Modify 'eslint' implementation to work with the mozlint framework, https://reviewboard.mozilla.org/r/74284/#review72160 ::: tools/lint/eslint.lint:273 (Diff revision 1) > - npm_path = self.get_node_or_npm_path("npm") > + npm_path = get_node_or_npm_path("npm") > if not npm_path: > return 1 > > - if self.eslint_module_has_issues(): > - install = self._prompt_yn("\nContinuing will automatically fix " > + if eslint_module_has_issues(): > + eslint_setup() This seems to mean that if there are any issues, we'll start making changes without ask for confirmation? I suppose it's for the best to do it automatically... In the past, confirmation was more important because we are altering packages at the system level, but now everything should be contained within the Gecko checkout. So, I guess it seems fine to make this change.

Comment on attachment 8784956 [details] Bug 1258341 - Modify 'eslint' implementation to work with the mozlint framework, Looks reasonable to me!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10) > This seems to mean that if there are any issues, we'll start making changes > without ask for confirmation? > > I suppose it's for the best to do it automatically... In the past, > confirmation was more important because we are altering packages at the > system level, but now everything should be contained within the Gecko > checkout. > > So, I guess it seems fine to make this change. Yeah that was my thought as well. By the time the develop finishes reading the prompt, we could have just had the modules updated already. But there's also a second more practical reason for it.. This code is now being run under a python 'multiprocessing' process, and raw_input doesn't work there. I do have plans to improve how the 'setup' of linters is handled, but was hoping to punt on it for a little longer.

Comment on attachment 8784958 [details] Bug 1258341 - Merge eslint-plugin-mozilla docs with the main linting docs, https://reviewboard.mozilla.org/r/74288/#review72180

Comment on attachment 8784956 [details] Bug 1258341 - Modify 'eslint' implementation to work with the mozlint framework, This was something I wanted to do from day one of introducing eslint... love it!

Comment on attachment 8784956 [details] Bug 1258341 - Modify 'eslint' implementation to work with the mozlint framework, Ryan and Mike have already answered. Don't think I can add more here.

Comment on attachment 8784530 [details] Bug 1258341 - [mozlint] Add ability to forward miscellaneous command line arguments to the underlying linter, https://reviewboard.mozilla.org/r/73950/#review73232

Comment on attachment 8784955 [details] Bug 1258341 - Literally copy all 'eslint' related functionality from tools/lint/mach_commands.py to tools/lint/eslint.lint, https://reviewboard.mozilla.org/r/74282/#review73234 Thanks, I appreciate this extra commit being added. But ya, definitely fold before landing.

Comment on attachment 8784956 [details] Bug 1258341 - Modify 'eslint' implementation to work with the mozlint framework, https://reviewboard.mozilla.org/r/74284/#review73238 LGTM :) ::: tools/lint/eslint.lint:254 (Diff revision 1) > > - if choice in ('n', 'no'): > - return False > > - print('Must reply with one of {yes, no, y, n}.') > +def lint(paths, binary=None, fix=None, setup=None, **lintargs): > + '''Run eslint.''' `"""`

Comment on attachment 8784957 [details] Bug 1258341 - Rewrite eslint taskcluster task to use |mach lint| instead of |mach eslint|, https://reviewboard.mozilla.org/r/74286/#review73248

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/64ed3dc696df [mozlint] Add ability to forward miscellaneous command line arguments to the underlying linter, r=smacleod https://hg.mozilla.org/integration/autoland/rev/f5c7a3d60bd9 Modify 'eslint' implementation to work with the mozlint framework, r=smacleod https://hg.mozilla.org/integration/autoland/rev/a2518f96d221 Rewrite eslint taskcluster task to use |mach lint| instead of |mach eslint|, r=smacleod https://hg.mozilla.org/integration/autoland/rev/ec7de1aac81c Merge eslint-plugin-mozilla docs with the main linting docs, r=miker

https://hg.mozilla.org/mozilla-central/rev/64ed3dc696df https://hg.mozilla.org/mozilla-central/rev/f5c7a3d60bd9 https://hg.mozilla.org/mozilla-central/rev/a2518f96d221 https://hg.mozilla.org/mozilla-central/rev/ec7de1aac81c