Closed Bug 1426266 Opened 6 years ago Closed 6 years ago

Enable or disable new eslint rules

Categories

(Firefox :: New Tab Page, enhancement, P3)

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Iteration:
60.1 - Jan 29
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file)

Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/3948
52 bytes, text/x-github-pull-request
Details | Review 
From https://github.com/mozilla/activity-stream/pull/3909#issuecomment-352897818

Here's the current state of new eslint rules (that we don't have set) and the number of errors in the current code:

https://eslint.org/docs/rules/array-bracket-newline 79 fixable
https://eslint.org/docs/rules/array-element-newline 473 fixable
https://eslint.org/docs/rules/capitalized-comments 355 fixable
https://eslint.org/docs/rules/class-methods-use-this 57
https://eslint.org/docs/rules/func-name-matching 1
https://eslint.org/docs/rules/function-paren-newline 178 fixable
https://eslint.org/docs/rules/implicit-arrow-linebreak 12 fixable
https://eslint.org/docs/rules/indent 18880 fixable
https://eslint.org/docs/rules/line-comment-position 31
https://eslint.org/docs/rules/lines-between-class-members 109 fixable
https://eslint.org/docs/rules/multiline-comment-style 196 fixable
https://eslint.org/docs/rules/no-multi-assign 3
https://eslint.org/docs/rules/prefer-destructuring 89
https://eslint.org/docs/rules/prefer-promise-reject-errors 1
https://eslint.org/docs/rules/require-await 17
https://eslint.org/docs/rules/sort-keys 961
                          
Below have no errors currently:                                           
https://eslint.org/docs/rules/for-direction
https://eslint.org/docs/rules/getter-return
https://eslint.org/docs/rules/no-await-in-loop
https://eslint.org/docs/rules/no-buffer-constructor
https://eslint.org/docs/rules/no-restricted-properties
https://eslint.org/docs/rules/nonblock-statement-body-position
https://eslint.org/docs/rules/padding-line-between-statements
https://eslint.org/docs/rules/prefer-numeric-literals
https://eslint.org/docs/rules/semi-style
https://eslint.org/docs/rules/switch-colon-spacing
https://eslint.org/docs/rules/symbol-description
https://eslint.org/docs/rules/template-tag-spacing
k88hudson, any thoughts on each of these?

(In reply to Ed Lee :Mardak from comment #0)
> https://eslint.org/docs/rules/array-bracket-newline 79 fixable
We can make this "consistent" instead of requiring newline inside every array.

> https://eslint.org/docs/rules/array-element-newline 473 fixable
Probably skip to not require newline between each element.

> https://eslint.org/docs/rules/capitalized-comments 355 fixable
Probably skip as it would capitalize method comment blocks, e.g.,
/* Foo */
function foo() {}

> https://eslint.org/docs/rules/class-methods-use-this 57
Probably skip to not needing to convert non-this methods to static.

> https://eslint.org/docs/rules/func-name-matching 1
The 1 failure is for a { test: function test_name() } where "test_name" is displayed when running mochitest, so probably just disable for tests.

> https://eslint.org/docs/rules/function-paren-newline 178 fixable
Probably skip as newlines inside a function call probably shouldn't be always, and even with consistent, it would make some jsx closing brace be hidden at the end of the line, e.g.,
{prefs.map(pref =>
  (<Pref…
    … />))}

vs the current
{prefs.map(pref =>
  (<Pref…
    … />)
)}

> https://eslint.org/docs/rules/implicit-arrow-linebreak 12 fixable
Probably skip as putting a new line after => mostly on the available line length.

> https://eslint.org/docs/rules/indent 18880 fixable
Probably skip as at least there has been some churn… although looks like some rules depend on this to cleanly autofix. E.g., some of the newline rules above would autofix by just inserting a newline without any leading spaces on the new line.

> https://eslint.org/docs/rules/line-comment-position 31
Probably skip as some line comments are nice to be inline vs above?

> https://eslint.org/docs/rules/lines-between-class-members 109 fixable
Sure? Yay autofix.

> https://eslint.org/docs/rules/multiline-comment-style 196 fixable
Mmm.. We do have some multi-line // blocks. But that seems okay…

> https://eslint.org/docs/rules/no-multi-assign 3
Fix the 3 errors and turn on? Only used in TopStoriesFeed:
body._timestamp = this.storiesLastUpdated = Date.now();

> https://eslint.org/docs/rules/prefer-destructuring 89
This will require a bit of manual fixing, but should be simple. Looks like the common cases:
const props = this.props; // const {props} = this;
const letterFallback = title[0]; // const [letterFallback] = title;

> https://eslint.org/docs/rules/prefer-promise-reject-errors 1
Ignore the 1 error and turn on? Only used in snippets test:
global.fetch.returns(Promise.reject({status: 400}));

> https://eslint.org/docs/rules/require-await 17
Mmm.. Sometimes just consistently using `async function()` is nice for tests, but we have seen some accidental errors / confusion for unnecessarily using async.

> https://eslint.org/docs/rules/sort-keys 961
Too many to manually fix. ;)

> Below have no errors currently:                                           
> https://eslint.org/docs/rules/for-direction
Sure.

> https://eslint.org/docs/rules/getter-return
Sure.

> https://eslint.org/docs/rules/no-await-in-loop
I suppose there could be some desired sequential async behavior that could be whitelisted.

> https://eslint.org/docs/rules/no-buffer-constructor
I don't think we use Buffer?

> https://eslint.org/docs/rules/no-restricted-properties
Eh. Turn on with empty?

> https://eslint.org/docs/rules/nonblock-statement-body-position
Sure. We require blocks for single-line anyway I think…

> https://eslint.org/docs/rules/padding-line-between-statements
Eh. Looks complex and does nothing with no configuration.. Just turn on empty?

> https://eslint.org/docs/rules/prefer-numeric-literals
Sure.

> https://eslint.org/docs/rules/semi-style
Sure.

> https://eslint.org/docs/rules/switch-colon-spacing
Sure.

> https://eslint.org/docs/rules/symbol-description
Sure? We don't use Symbol().

> https://eslint.org/docs/rules/template-tag-spacing
Sure? We haven't really used template strings…
Flags: needinfo?(khudson)
Iteration: 1.25 → 1.26
Iteration: 1.26 → 59.4 - Jan 15
Blocks: 1429894
No longer blocks: 1429894
> > https://eslint.org/docs/rules/array-bracket-newline
> "consistent" seems like the right call to me
> 
> > https://eslint.org/docs/rules/array-element-newline
> I would skip rhis
> 
> > https://eslint.org/docs/rules/capitalized-comments
> Yes, let's skip
> 
> > https://eslint.org/docs/rules/class-methods-use-this
> Yes, let's skip.
> 
> > https://eslint.org/docs/rules/func-name-matching
> Personally I would skip this one, although exceptions are infrequent could add ignore comments
> 
> > https://eslint.org/docs/rules/function-paren-newline
> I would skip
> 
> > https://eslint.org/docs/rules/implicit-arrow-linebreak
> Yes, skip
> 
> > https://eslint.org/docs/rules/indent
> I'm confused as to why this one would have so many failures
> 
> > https://eslint.org/docs/rules/line-comment-position
> Yes, skip
> 
> > https://eslint.org/docs/rules/lines-between-class-members
> This is fine sure
> 
> > https://eslint.org/docs/rules/multiline-comment-style
> I guess this would be fine, but I don't really particularly care about making it consistent 
> 
> > https://eslint.org/docs/rules/no-multi-assign
> Sure
> 
> > https://eslint.org/docs/rules/prefer-destructuring
> Eh, why not
> 
> > https://eslint.org/docs/rules/prefer-promise-reject-errors 
> Sure, and you could probably just modify the test to send an error
> 
> > https://eslint.org/docs/rules/require-await
> I'd be OK with this, perhaps turn it off for tests?

> > https://eslint.org/docs/rules/sort-keys
> Let's not
> 
> > Below have no errors currently:                                           
> > https://eslint.org/docs/rules/for-direction
> > https://eslint.org/docs/rules/getter-return
Yes


> > https://eslint.org/docs/rules/no-await-in-loop
I don't really agree with the premise of this one?
> 
> > https://eslint.org/docs/rules/no-buffer-constructor
> Yeah this one is kind of N/A
> 
> > https://eslint.org/docs/rules/no-restricted-properties
> I don't really see the need for it
> 
> > https://eslint.org/docs/rules/nonblock-statement-body-position
> Yeah, this is fine
> 
> > https://eslint.org/docs/rules/padding-line-between-statements
> I would skip this one
> 
> > https://eslint.org/docs/rules/prefer-numeric-literals
> > https://eslint.org/docs/rules/semi-style
> > https://eslint.org/docs/rules/switch-colon-spacing
> > https://eslint.org/docs/rules/symbol-description
> > https://eslint.org/docs/rules/template-tag-spacing
> Yes
Flags: needinfo?(khudson)
(In reply to Kate Hudson :k88hudson from comment #2)
> > https://eslint.org/docs/rules/indent
> I'm confused as to why this one would have so many failures
I believe a big part is jsx stuff.
Iteration: 59.4 - Jan 15 → 60.1 - Jan 29
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/2466bf2e1f9978d1da517d700e4458d67eefca38
Merge pull request #3948 from Mardak/b1426266-eslint

Fix Bug 1426266 - Enable or disable new eslint rules
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1434116
https://hg.mozilla.org/mozilla-central/rev/97b3915d56c4
status-firefox60: --- → fixed
Target Milestone: --- → Firefox 60
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: