Closed Bug 1460856 Opened 6 years ago Closed 6 years ago

[mozlint] Don't fail for warnings and make them silent by default

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(4 files)

As of now, we cause |mach lint| to return a failure for both errors and warnings. This was done intentionally so that we don't have infractions slipping through that start showing up on unrelated changes. I.e we want to make sure that whoever caused the failure is on the hook for fixing it.

However, I think there is space for a lint infraction that is "best practice", but not something so bad that we want to prevent it outright/cause backouts. One example is the spellcheck linter. Not only is it annoying to be backed out over a typo in a comment, but there is a lot of jargon that it might *think* is a typo, which is actually not. For these class of lint errors, one could use |mach lint| after the fact to help hunt down and fix these errors at their leisure.

I still believe it's important to keep the default output of |mach lint| clean. I.e, no errors == no backout. I propose:

1. We hide "warning" level lint infractions by default. You would have to run ./mach lint --level=warning (or similar) to see them.

2. We don't fail for "warning" level infractions. I.e they don't fail the task and they don't block the push/commit. We *could* still have them show up on mozreview as I believe the reviewbot does a diff of errors before and after each commit to tell exactly what that commit introduced.

3. We convert all existing linters to use errors exclusively for things we want to catch. We have already done this for eslint, and doing it for other linters wouldn't be too hard either.

This set up should make everyone happy. We block bad lint infractions from landing, while at the same time allowing us to enable a less strict set of rules that only run on review (or when requested). All while keeping the default output clean.
Priority: -- → P3
I'm going to see if I can chip away at this.

A slight modification to the original proposal, warnings will still return 1 when they're enabled but they won't cause failures when suppressed (which will still be the default behaviour).
Assignee: nobody → ahal
Status: NEW → ASSIGNED
Soon, warnings will be suppressed by default and won't causes a failure.
Therefore to prevent loss of coverage, we need to make sure that any
lint warning that causes a failure today, needs to be converted to an
error so it keeps failing tomorrow.

Depends on D3819
Since the last change converted everything to use errors, this patch should
be a no-op for now. However as we move forward (and potentially decide that
certain types of lint issues should result in a backout), this feature will
start seeing more and more use.

Depends on D3820
Comment on attachment 9002568 [details]
Bug 1460856 - [mozlint] Suppress warnings by default

Mark Banner (:standard8) has approved the revision.
Attachment #9002568 - Flags: review+
Comment on attachment 9003184 [details]
Bug 1460856 - [mozlint] Display suppressed warnings count in the summary and stylish formatters

Mark Banner (:standard8) has approved the revision.
Attachment #9003184 - Flags: review+
Comment on attachment 9002568 [details]
Bug 1460856 - [mozlint] Suppress warnings by default

Sylvestre Ledru [:sylvestre] has approved the revision.
Attachment #9002568 - Flags: review+
Version: Version 3 → unspecified
Comment on attachment 9002566 [details]
Bug 1460856 - [mozlint] Encapsulate all result state in a ResultSummary class

Sylvestre Ledru [:sylvestre] has approved the revision.
Attachment #9002566 - Flags: review+
Whiteboard: [leave-open]
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88fa28d336fa
[mozlint] Encapsulate all result state in a ResultSummary class r=sylvestre
Comment on attachment 9002567 [details]
Bug 1460856 - [mozlint] Stop using warnings in all current linters

Mark Banner (:standard8) has approved the revision.
Attachment #9002567 - Flags: review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38549f3a5187
[mozlint] Stop using warnings in all current linters r=Standard8
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f664646a2e7c
[mozlint] Suppress warnings by default r=Standard8,sylvestre
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b604e8837547
[mozlint] Display suppressed warnings count in the summary and stylish formatters r=Standard8
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/88fa28d336fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1487425
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: