1499089 - mach lint --warnings fails when manually linting ignored files

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Gabriele Svelto [:gsvelto]]

./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).

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1499089 - Handle warnings without a line number correctly

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Gabriele Svelto [:gsvelto]]

I've updated the patch with the suggested fix.

Comment 4

[Andrew Halberstadt [:ahal]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/188475039aa5
Handle warnings without a line number correctly r=ahal

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/188475039aa5

Comment 7

[Mark Banner (:standard8)]

(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.

Comment 8

[Andrew Halberstadt [:ahal]]

Good point, I guess instead of capturing it and stuffing it into an Issue object we could just print the line to stdout.