[mozlint] Improve the default behaviour if no file paths are specified
source link: https://bugzilla.mozilla.org/show_bug.cgi?id=1369784
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
[mozlint] Improve the default behaviour if no file paths are specified
Categories
(Firefox Build System :: Lint and Formatting, enhancement, P2)
Tracking
(firefox88 fixed)
88 Branch
People
(Reporter: ahal, Assigned: nisargkarun1997)
References
Details
Currently if you run |mach lint| without any arguments and you happen to be in $topsrcdir, it will run all linters against the entire tree. This takes a really long time (~20 minutes currently), and makes it look like the command has hung. Furthermore, I don't think it's ever really the desired behaviour. Since lint errors shouldn't be able to sneak into the tree, --outgoing should contain the only files with potential errors in them. Besides, if a developer actually wanted to lint the whole tree, they could simply run: ./mach lint . There have been a few reports of --outgoing not working properly, especially with git-cinnabar. Before making it a default, we'd need to iron out any issues with it and make sure it has good test coverage.
Will outgoing also test files if configurations are changed? e.g. it is quite common for new rules to be added to the recommended.js for eslint (or items to one of the .eslintrc.js files), and that's a situation where we do need to have it run against all relevant directories. If --outgoing isn't run against all files with those changes, then it will likely lead to people missing items (I've also just realised this is an existing issue with the eslint hooks, though should be fairly easy to solve).
Good point, it wouldn't do that. Maybe we could add a 'config_files' attribute to the lint definition that the --outgoing mode would use to run the whole thing if configuration changed.
(In reply to Andrew Halberstadt [:ahal] from comment #2) > Good point, it wouldn't do that. Maybe we could add a 'config_files' > attribute to the lint definition that the --outgoing mode would use to run > the whole thing if configuration changed. How about some sort of function or regexp? Pass it the list of files, and get back if it needs to do everything (or maybe even the list of sub-directories to do).
Support files have been supported for a long time... we should really fix this :). Options for a default include:
--outgoing--outgoing --workdir*(lints same files as current default but does so with more parallelization)
I'm on the fence about which default is better. I think 1) or 2) will solve the most common cases (lint the things that I touched) and will also be the fastest. Though it might lead to some suprising UX. E.g, if a developer has mach on their path, they could do:
$ cd devtools
$ mach lint
And be surprised that things outside of devtools were linted. So 3) seems like it would provide the most consistent command line UX. It also provides backwards compatibility in case anyone was invoking mach lint from a script/editor/etc.
I'm on the fence, in some ways I think 3) would result in the least surprises for developers. Generally if we run a command like that, we expect it to apply to the whole directory (even if that's madness for all of lint).
If we did one of the others, I'd vote for 2) since I think that makes most sense.
If we defaulted to outgoing we could get it to print out the fact it is doing outgoing, and say how to override it. Possibly ./mach eslint would still default to the whole tree in that case, as it isn't as bad as ./mach lint on the whole tree, and probably intentional.
Ok yeah, I think it's best to go with 3) for now. We can always switch it later if we find there's a need. I'll see if I can get this fixed sometime during the all-hands.
In bug 1621322, :sfink was surprised that running mach lint without arguments didn't lint his touched files. I think we should reconsider here and run --outgoing --workdir by default.
One possible hybrid is to only use this behaviour if we're running from the root. So e.g,:
$ cd topsrcdir
$ mach lint # only lints outgoing/workdir
$ cd devtools
$ mach lint # lints entire devtools directory
Not sure if this makes things more or less confusing.. but a blank mach lint from the root of the repo is completely unusable, so IMO confusing behaviour is better than the status quo.
So to recap we should lint outgoing/workdir when all of the following are true:
- No paths or linters were specified (i.e we are running all linters on all paths)
- CWD == lint.root
- self.vcs is not None
When we do this substitution, we should also print a warning explaining what is happening and how to work around it (e.g by passing in .)
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK