1756224 - [perfdocs] Reviewbot comments are providing incorrect usage information

Status: RESOLVED FIXED

(Testing :: Performance, defect, P2)

Description

[Greg Mierzwinski [:sparky]]

The recommended command for the perfdocs linter is currently broken/outdated in multiple areas because, with little to no warning, the linter usage was changed (see bug in regressed by): https://searchfox.org/mozilla-central/search?q=lint&path=perfdocs&case=false&regexp=false

It's unclear why we can't have the default set to */. for perfdocs either, since it always runs with this setting. I see here that the paths should default to cwd as well so it's really unclear what's going on here or if the mozlint documentation is now outdated as well: https://searchfox.org/mozilla-central/source/python/mozlint/mozlint/cli.py#23

Comment 1

[Mitchell Hentges [:mhentges] 🦀]

The recommended command for the perfdocs linter is currently broken/outdated in multiple areas because, with little to no warning

Hey, sorry about the regression here, I was wondering who all it would impact and I didn't consider the performance crew 😅

I see here that the paths should default to cwd as well so it's really unclear what's going on here or if the mozlint documentation is now outdated

The docs are outdated, by default it now uses --outgoing --workdir.

It's unclear why we can't have the default set to */. for perfdocs either, since it always runs with this setting

Hmm, this is a bit ambiguous, but I'll answer both interpretations :)

1. Why can't ./mach lint have a perfdocs-specific default of using */.?

./mach lint doesn't know who is actively calling it (except by reaching around for environment variables, which is brittle). It'd be better for perfdocs to explicitly declare that it wants the full directory to be linted.

2. Why can't we change perfdocs to, by default, call ./mach lint with */.?

I think that this is the correct solution 👍

Comment 2

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Bug 1756224: Update MozlintParser docs about default `paths`

Updates parser documentation to clarify that, if no paths are
provided, then only those discovered by --outgoing and --workdir
are linted.

Comment 3

[Mitchell Hentges [:mhentges] 🦀]

Attachment: Bug 1756224: Update references to perfdocs linter to explicitly set path

Ideally, the perfdocs linter suggestion to --fix issues would take
into account the paths that were provided, rather than printing the
sure-fire "lint and fix all" solution.

Actually, on further consideration, theoretically such a recommendation
should appear at the ./mach lint level, and not be linter-specific. Oh
well, that's a future improvement :)

---

This patch updates the perfdocs documentation and suggestions to
successfully operate in the same manner that they did before
bug 1753701. One line was added to the run_perfdocs() docstring to
hint towards the "only lint changed files" approach.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1753701

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f71993d94b03
Update MozlintParser docs about default `paths` r=ahal
https://hg.mozilla.org/integration/autoland/rev/f73b04feffdd
Update references to perfdocs linter to explicitly set path r=sparky

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f71993d94b03
https://hg.mozilla.org/mozilla-central/rev/f73b04feffdd