Closed Bug 1499089 Opened 11 months ago Closed 11 months ago

mach lint --warnings fails when manually linting ignored files

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

Details

Attachments

(1 file)

./mach lint --warnings intl/locale/tests/unit/test_localeService.js

fails with the following stack:

  File "/home/gsvelto/projects/mozilla-central/tools/lint/mach_commands.py", line 55, in lint
    return cli.run(*runargs, **lintargs)
  File "/home/gsvelto/projects/mozilla-central/python/mozlint/mozlint/cli.py", line 184, in run
    out = formatter(result).encode(sys.stdout.encoding or 'ascii', 'replace')
  File "/home/gsvelto/projects/mozilla-central/python/mozlint/mozlint/formatters/stylish.py", line 72, in __call__
    for err in sorted(errors, key=lambda e: (int(e.lineno), int(e.column or 0))):
  File "/home/gsvelto/projects/mozilla-central/python/mozlint/mozlint/formatters/stylish.py", line 72, in <lambda>
    for err in sorted(errors, key=lambda e: (int(e.lineno), int(e.column or 0))):

This is because the code handling linters excepts warnings to always have a line numbers but in this case the warning doesn't have one (because it only mentions the file is ignored).
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
This is also an issue when using --outgoing/--workdir (since those pass in specific files too).

I think I'd prefer if mozlint's model made line numbers mandatory. It's not just the formatters that depend on it, for example |mach lint --edit| depends on the line number to open the error up in an editor as well.

I'd say this is more of a bug in the eslint <-> mozlint glue code here:
https://searchfox.org/mozilla-central/source/tools/lint/eslint/__init__.py#112

We could set the default to 0 (or maybe 1?) there in that `.get()` call. Another alternative would be to just discard those "ignored file" warnings outright. I'd prefer discarding myself since that will make eslint consistent with all our other linters, though I'd like to defer to Standard8 on that.

Mark, basically is that "file ignored" warning that eslint emits ever useful? Or can we just get rid of it.
Flags: needinfo?(standard8)
I've updated the patch with the suggested fix.
Thanks, I think I'll keep the needinfo out of curiosity, but we can tackle that particular question in a follow-up bug. Let's land this for now and get this unbroken.
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/188475039aa5
Handle warnings without a line number correctly r=ahal
https://hg.mozilla.org/mozilla-central/rev/188475039aa5
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> We could set the default to 0 (or maybe 1?) there in that `.get()` call.
> Another alternative would be to just discard those "ignored file" warnings
> outright. I'd prefer discarding myself since that will make eslint
> consistent with all our other linters, though I'd like to defer to Standard8
> on that.
> 
> Mark, basically is that "file ignored" warning that eslint emits ever
> useful? Or can we just get rid of it.

I think as a warning it isn't useful. However, I think that we should be outputting either an error or some form of indication if a person attempts to lint an ignored file.

So if I do `./mach lint -l eslint intl/locale/tests/unit/test_localeService.js` I should get some indication that it is ignored.

Otherwise we run the risk of letting people think a file is linted, when it isn't.
Flags: needinfo?(standard8)
Good point, I guess instead of capturing it and stuffing it into an Issue object we could just print the line to stdout.
You need to log in before you can comment on or make changes to this bug.