Skip to content
This repository has been archived by the owner. It is now read-only.

Pass --warnings into mozlint #1447

Closed
ahal opened this issue Aug 28, 2018 · 5 comments
Closed

Pass --warnings into mozlint #1447

ahal opened this issue Aug 28, 2018 · 5 comments

Comments

@ahal
Copy link
Member

@ahal ahal commented Aug 28, 2018

Once https://bugzilla.mozilla.org/show_bug.cgi?id=1460856 lands, it would be nice if the reviewbot enabled warnings by default.

Warnings wouldn't cause the patch to be backed out, but are still things that we should usually try to fix. So when a patch author gets a 'warning' level issue, they should fix it, but would be totally within their rights to ignore it instead.

This will allow us to enable things like the 'codespell' linter on a much larger set of paths, without needing to worry about false positives as much. Of course, since warnings don't get backed out, we'd need to make sure the reviewbot only displays warnings introduced by the patch. But I think it already handles this.

@marco-c marco-c changed the title shipit_static_analysis: Pass --warnings into mozlint Pass --warnings into mozlint Sep 4, 2018
@jarkyll
Copy link

@jarkyll jarkyll commented Dec 9, 2018

@ahal @La0 Can I take this issue up as my first bug?

@marco-c
Copy link
Collaborator

@marco-c marco-c commented Dec 10, 2018

@ahal how noisy are warnings? Right now we are using the in_patch heuristic, where we report issues if they are on a line which was modified by a patch. In the future, we will switch to the before\after heuristic, where we will report issues that are new (that is, not reporting issues that were there both before and after the patch).
Can we enable the warnings with the in_patch heuristic, or do we need to wait for before\after?

@marco-c
Copy link
Collaborator

@marco-c marco-c commented Dec 10, 2018

N.B.: We were also considering keeping to report in_patch issues even if they are not new, using before\after only to increase the number of issues we report. Would showing warnings be too noisy? If so, we could specify a set of checkers for which we allow in_patch and a set for which we only allow before/after.

@ahal
Copy link
Member Author

@ahal ahal commented Feb 21, 2019

I missed the questions here, but I don't believe warnings are currently noisy at all. As we add more things at the warning level however, this could very easily change.

@Pike
Copy link

@Pike Pike commented Feb 25, 2019

sylvestre added a commit that referenced this issue Feb 25, 2019
Closes #1447
@La0 La0 closed this in #1907 Feb 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants