Refactor |mach eslint| to use mozlint library

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Assignee

Description

3 years ago
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.
Assignee

Updated

3 years ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Depends on: 1271734
Assignee

Comment 2

3 years ago
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/
Attachment #8759320 - Flags: review?(mratcliffe)
Assignee

Updated

3 years ago
Attachment #8732872 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8759320 - Attachment is obsolete: true
Attachment #8759320 - Flags: review?(mratcliffe)
Component: General → ESLint
Assignee

Updated

3 years ago
Summary: Refactor |mach eslint| to be based on mozlint library from bug 1230962 → Refactor |mach eslint| to use mozlint library
Assignee

Updated

3 years ago
Depends on: 1294802
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 9

3 years ago
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!
Attachment #8784956 - Flags: feedback?(pbrosset)
Attachment #8784956 - Flags: feedback?(mratcliffe)
Attachment #8784956 - Flags: feedback?(jryans)

Comment 10

3 years ago
mozreview-review
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!
Attachment #8784956 - Flags: feedback?(jryans) → feedback+
Assignee

Comment 12

3 years ago
(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
Attachment #8784958 - Flags: review?(mratcliffe) → review+
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!
Attachment #8784956 - Flags: feedback?(mratcliffe) → feedback+
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.
Attachment #8784956 - Flags: feedback?(pbrosset)

Comment 16

3 years ago
mozreview-review
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
Attachment #8784530 - Flags: review?(smacleod) → review+

Comment 17

3 years ago
mozreview-review
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.
Attachment #8784955 - Flags: review?(smacleod) → review+

Comment 18

3 years ago
mozreview-review
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.'''

`"""`
Attachment #8784956 - Flags: review?(smacleod) → review+

Comment 19

3 years ago
mozreview-review
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
Attachment #8784957 - Flags: review?(smacleod) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8784955 - Attachment is obsolete: true

Comment 24

3 years ago
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
Assignee

Updated

3 years ago
Depends on: 1299618

Updated

Last year
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.