-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: eslint object curly spacing #14162
Changes from all commits
c74fff3
b7ba73f
4459d3b
def16d4
1040d72
267992a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ test/fixtures | |
test/disabled | ||
test/tmp*/ | ||
tools/eslint | ||
tools/icu | ||
node_modules | ||
benchmark/tmp/ | ||
doc/**/*.js |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,16 +135,17 @@ rules: | |
}] | ||
no-tabs: error | ||
no-trailing-spaces: error | ||
object-curly-spacing: [error, always] | ||
one-var-declaration-per-line: error | ||
operator-linebreak: [error, after] | ||
quotes: [error, single, avoid-escape] | ||
semi: error | ||
semi-spacing: error | ||
space-before-blocks: [error, always] | ||
space-before-function-paren: [error, { | ||
"anonymous": "never", | ||
"named": "never", | ||
"asyncArrow": "always" | ||
anonymous: never, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is unrelated and should be done in another PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was done in response to: #14162 (comment) I understand it's not 100% the same property but opening another PR seems a little bureaucratic. It's a purely aesthetic change in line with the comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sebdeckers "bureaucratic" rules have their reasons ;) One of them is that keeping commits logically separated makes backporting a lot easier. Most of the changes from this PR are probably going to have numerous conflicts with release branches, while this one would have been cherry-picked cleanly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @aqrln in the general case, but in this case since it's most probably gonna be backported by cherry picking just the rules change, then running |
||
named: never, | ||
asyncArrow: always | ||
}] | ||
space-in-parens: [error, never] | ||
space-infix-ops: error | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can reduce the churn slightly and conform more closely to the house style by doing this instead:
That flags 2144 problems in the current code base, instead of the 2183 flagged with the PR as it is. That's not a huge reduction, but every little bit helps to make backporting less awful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% of these changes were made using
eslint --fix
automatically. Except for ~30 cases where the insertion of spaces then caused the line to grow longer than 80 characters, which I cleaned up manually following my own stylistic judgement and ensuring eslint passes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it's useful to do the automatic ones as one commit, and then the manual ones as a separate one. They'll get squashed on landing, but it means people can review the ones that weren't just an
eslint --fix
(which are probably the more interesting ones).