Closed
Bug 1264063
Opened 9 years ago
Closed 9 years ago
Disable noisy eslint rules untils they get fixed
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ochameau, Assigned: pbro)
References
Details
Attachments
(2 files, 4 obsolete files)
There is so many warnings now that we don't even see errors in Treeherder shortlog panel...
This is also very disturbing for contributors.
Even mozilla employees contributing to /devtools/ find this disturbing.
We should disable these rules until they get fixed.
Reporter | ||
Comment 1•9 years ago
|
||
Just disable them everywhere it is needed.
Attachment #8740625 -
Flags: review?(jryans)
Reporter | ||
Comment 2•9 years ago
|
||
Also fix this rule everywhere in /devtools/.
It is surprising to have this enabled and have so few warnings?
It seems to report all ContentTask usages... unless I miss something.
Attachment #8740626 -
Flags: review?(jryans)
Reporter | ||
Comment 3•9 years ago
|
||
If we do care about this rule, we should make it an error.
Attachment #8740627 -
Flags: review?(jryans)
Comment on attachment 8740625 [details] [diff] [review]
Disable noisy eslint warning in /devtools/
Review of attachment 8740625 [details] [diff] [review]:
-----------------------------------------------------------------
My preference would be to disable rules from devtools/.eslintrc until they can be enabled as an error (2).
In a previous bug, I believe :pbro said there's not much point in configuring rules as warnings, and currently it's only the plugins that do this. By avoiding warnings altogether, we get out of the bad state in Treeherder: either the rule is off or it's an error and must be fixed.
Attachment #8740625 -
Flags: review?(jryans) → review?(pbrosset)
Comment on attachment 8740626 [details] [diff] [review]
Fix the various no-cpow-in-tests warning in /devtools/
Review of attachment 8740626 [details] [diff] [review]:
-----------------------------------------------------------------
This doesn't seem right to me. Should we disable the rule? Or at least file a bug to fix it with ContentTask?
Mike, what do you think here?
Attachment #8740626 -
Flags: review?(jryans) → feedback?(mratcliffe)
Comment on attachment 8740627 [details] [diff] [review]
Toggle no-cpow-in-tests to error to prevent regressing it
Review of attachment 8740627 [details] [diff] [review]:
-----------------------------------------------------------------
Let's see what we determine about the previous no-cpow change.
Attachment #8740627 -
Flags: review?(jryans)
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8740625 [details] [diff] [review]
Disable noisy eslint warning in /devtools/
Review of attachment 8740625 [details] [diff] [review]:
-----------------------------------------------------------------
Yes, I don't see a point of having a rule if it's set to generate warnings. And I've talked to people multiple times about fixing this noise in the past. I entirely agree that this is confusing for everyone and that we should get rid of them. I don't really know why this hasn't been fixed already, especially because it's such a simple change.
What I would suggest here is:
- turn react/sort-comp to 2 (error) and fix the corresponding code, because it's such a simple change
- turn react/prop-types to 0 (disabled) in the main devtools/.eslintrc file because it requires more code and I'm not comfortable with it. I'd like someone working on responsive.html or about:debugging to do it. It should be quick because there aren't too many errors.
I'll upload a patch that does this in a minute.
Attachment #8740625 -
Flags: review?(pbrosset)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8740625 -
Attachment is obsolete: true
Attachment #8740848 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8740626 [details] [diff] [review]
Fix the various no-cpow-in-tests warning in /devtools/
Review of attachment 8740626 [details] [diff] [review]:
-----------------------------------------------------------------
So, unless we finally fix the CPOW rule to ignore ContentTask, we don't have any other options than to either disable the rule altogether, or disable lines of code as you did.
Fixing the rule should be easy. I'll take a look at it now.
Attachment #8740626 -
Flags: feedback?(mratcliffe)
Assignee | ||
Comment 10•9 years ago
|
||
This makes the CPOW rule ignore ContenTask.spawn, and turns the rule to error so it's not just warning noise in the logs anymore.
Now, of course, this can't catch things like
let location = yield spawnViewportTask(ui, {}, function* () {
return content.location.href;
});
where spawnViewportTask actually wraps ContentTask.spawn. Fixing this would be a whole lot more difficult. So, I suggest doing this for these cases:
let location = yield spawnViewportTask(ui, {}, function* () {
return content.location.href; // eslint-disable-line
});
which is essentially what you did in the first patch Alex, but hopefully this should be reduced to only a few places instead of everywhere we use ContentTask.
Attachment #8740626 -
Attachment is obsolete: true
Attachment #8740627 -
Attachment is obsolete: true
Attachment #8740849 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 11•9 years ago
|
||
Sorry for stealing this bug :( but this should get rid of all warnings, so hopefully it's for the best :)
I'm wondering if we should not simply disable the CPOW rule after all ... we now have CPOW usage checks in tests, so I don't know that this rule is really all that helpful now.
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8740848 [details] [diff] [review]
Bug_1264063_-_1_-_Disable_prop-types_eslint_rule_a.diff
Review of attachment 8740848 [details] [diff] [review]:
-----------------------------------------------------------------
A man of action!
I like when throwing patches ends up receiving even better ones ;)
Attachment #8740848 -
Flags: review?(poirot.alex) → review+
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8740849 [details] [diff] [review]
Bug_1264063_-_2_-_Make_the_CPOW_rule_log_errors_an.diff
Review of attachment 8740849 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Patrick Brosset [:pbro] from comment #11)
> Sorry for stealing this bug :( but this should get rid of all warnings, so
> hopefully it's for the best :)
>
> I'm wondering if we should not simply disable the CPOW rule after all ... we
> now have CPOW usage checks in tests, so I don't know that this rule is
> really all that helpful now.
Does this check works in mochitest browser? Does it make the test fail and reject landing?
It does work in optimized build, i.e. there is no need to be un debug to have this check?
If the answer is yes to all these questions. Then yes, let just make eslint faster.
If you modify a test, you will at least run it locally and get the runtime exception,
so eslint doesn't save you from running something here.
::: testing/eslint-plugin-mozilla/lib/rules/no-cpows-in-tests.js
@@ +45,5 @@
> + }
> +
> + function isInContentTask(node) {
> + while (node.parent) {
> + var expression = node.argument ? node.argument.callee : node;
I'm not a eslint expert so I don't really follow this.
But the while() loop seems possibly expensive if there is many parent.
Attachment #8740849 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 16•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46201/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46201/
Attachment #8741103 -
Flags: review?(dtownsend)
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8740849 [details] [diff] [review]
Bug_1264063_-_2_-_Make_the_CPOW_rule_log_errors_an.diff
Took another approach to fix the no-cpow rule to avoid the while loop. Simply by adding AST listeners at the right moments we can track when we enter and exit a ContentTask.spawn execution and therefore ignore CPOW warnings then.
Attachment #8740849 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
New pending try build (ESLint job successful, which is what matters here): https://treeherder.mozilla.org/#/jobs?repo=try&revision=61df5e57d4f3
Comment 19•9 years ago
|
||
Comment on attachment 8741103 [details]
MozReview Request: Bug 1264063 - 2 - Make the CPOW rule log errors and ignore ContentTask.spawn; r=Mossop
https://reviewboard.mozilla.org/r/46201/#review42727
Attachment #8741103 -
Flags: review?(dtownsend) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Assignee: poirot.alex → pbrosset
Status: NEW → ASSIGNED
Comment 21•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 22•9 years ago
|
||
reopen as a 2nd patch need landing
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•9 years ago
|
Comment 23•9 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Blocks: 1262407
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•