Closed Bug 1503599 Opened 1 year ago Closed 1 year ago
Error: int() argument must be a string or a number, not 'None Type' on running WPT linter
47 bytes, text/x-phabricator-request
|Details | Review|
When the WPT linter produces a lint error without a line number, mozlint passes None to the built-in int() coercion function, resulting in the warning seen below. This can be reproduced by creating a file with some linting mistakes in it and running "./mach lint -lwpt": > % echo 'print "foo"' >testing/web-platform/tests/webdriver/foo.py > % ./mach lint -lwpt testing/web-platform/tests/webdriver/foo.py > Traceback (most recent call last): > File "/home/ato/src/gecko/python/mozlint/mozlint/roller.py", line 39, in _run_worker > res = func(paths, config, **lintargs) or  > File "/home/ato/src/gecko/python/mozlint/mozlint/types.py", line 48, in __call__ > return self._lint(paths, config, **lintargs) > File "/home/ato/src/gecko/python/mozlint/mozlint/types.py", line 132, in _lint > return func(files, config, **lintargs) > File "/home/ato/src/gecko/tools/lint/wpt/wpt.py", line 47, in lint > proc.returncode)) > File "/home/ato/src/gecko/python/mozlint/mozlint/result.py", line 133, in from_config > return Issue(**attrs) > File "/home/ato/src/gecko/python/mozlint/mozlint/result.py", line 85, in __init__ > self.lineno = int(lineno) > TypeError: int() argument must be a string or a number, not 'NoneType' > A failure occurred in the wpt linter. > ✖ 1 problem (0 errors, 0 warnings, 1 failure)
AIUI, this is happening because the wpt linter is linting the file name itself and therefore doesn't have a line number to give. Then mozlint is assuming that the line number is present with that call to `int` (which is a recent regression). This raises a somewhat fundamental question about what mozlint is and isn't. Do we want to encourage and make it easier for linters that don't have a line number? There is a lot of code that kind of assumes that there is (e.g formatters and opening errors in an editor are two examples of places that make this assumption). There are two options: 1) Make linters that don't use line numbers first class 2) Use a lineno of 0 to denote a file level issue and add better error checking to enforce int in mozlint I'm kind of tempted to just do 2 for now and investigate doing 1 properly in a follow-up bug. Solution 1 is a bit more work than I want to spend on this right now. Plus solution 2 would still be an improvement on the old (non-busted) behaviour where we were getting formatted output that looked like "None An error occurred (wpt)".
Assignee: nobody → ahal
Status: NEW → ASSIGNED
It seems perfectly reasonable to want to emit linter warnings about the general state a file is in, without pin-pointing the problem to a specific line in that file. For a file-level error in WPT, I would expect the following: > % ./mach lint -funix -lwpt testing/web-platform/tests/foo.py > testing/web-platform/tests/foo.py: this file does not conform to whatever WPT specific reason If I saw this instead, I would be confused: > testing/web-platform7tests/foo.py:0: this file does not conform to whatever WPT specific reason I guess I don’t exactly see why mozlint can’t take a NoneType and skip the line number coercion with int(), but if the proposal is to pass in 0 and have the formatter/output skip writing ":0", that would also satisfy my needs. I don’t have any particular opinion how this is fixed internally, for as long as the output that is present to users makes sense.
This assumes that a lineno of 0 denotes a "file-level" issue, e.g an issue associated with the filename. In the future it might be better to treat these "file-level" issues as first class citizens. This would involve updating things like formatters, editor integrations and review bot to not assume a lineno. This also adds better error checking for non-int values in mozlint along with a test.
Yeah, I think my main issue is one of time. It's not just formatters, it might affect the reviewbot or some editor integrations as well. Keeping at as an int just reduces the risk of failure. I'm definitely on board with the formatters detecting lineno=0 and omitting the ":0". I'm just saying I'm not going to be fixing that in this bug. Note that the previous behaviour was to print: testing/web-platform7tests/foo.py:None: this file does not conform to whatever WPT specific reason So at the very least ":0" is not any worse :). p.s I filed bug 1503606 as follow-up, though don't expect to have much time to work on it.
Attachment #9021552 - Attachment description: Bug 1503599 - [lint] Default lineno to 0 in wpt linter, r?jgraham → Bug 1503599 - [mozlint] Default lineno to 0 in result.Issue if unset, r?jgraham
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/9d9cc09cded0 [mozlint] Default lineno to 0 in result.Issue if unset, r=jgraham
This assumes that a lineno of 0 denotes a "file-level" issue, e.g an issue associated with the filename. In the future it might be better to treat these "file-level" issues as first class citizens. This would involve updating things like formatters, editor integrations and review bot to not assume a lineno.
You need to log in before you can comment on or make changes to this bug.