Eslint: Restore curly rule with prettier #61204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 1.74 MB ℹ️ View Unchanged
|
sirreal
force-pushed
the
fix/eslint-prettier-and-curly-rule
branch
from
4692589
to
d26888e
Compare
gziolo
reviewed
Apr 29, 2024
| // > This rule requires certain options. | ||
| // > … | ||
| // > If you like this rule, it can be used just fine with Prettier as long as you don’t use the "multi-line" or "multi-or-nest" option. | ||
| curly: [ 'error', 'all' ], |
There was a problem hiding this comment.
It was likely a conscious decision to let Prettier format everything and remove all the opinions from ESLint. More context:
https://make.wordpress.org/core/2019/12/09/proposed-javascript-coding-standards-revisions-for-prettier-compatibility/
It's probably fine to change it for Gutenberg, but from the standpoint of WordPress projects that would be a breaking change that would require communicating with the community on Make Core blog.
There was a problem hiding this comment.
Yep, I'll make this a breaking change.
I'm digging in history, but I suspect disabling this rule when prettier is enabled was an oversight.
Edit:
- Added to eslint-config-prettier in 1.6.0
There was a problem hiding this comment.
All the prettier eslint changes started in #18048 and I believe curly has been effectively disabled since then. I don't see discussion of this particular point so I'm not sure whether it was intentional or not.
As far as I can tell, we intended to remove the eslint rules that explicitly conflict with prettier, and curly may conflict but never did in our case, and it was disabled without considering it specifically.
There was a problem hiding this comment.
curly is not a formatting option and thus not really something Prettier covers, so it's not really a conflicting rule (unless the multi-line option is chosen). IMO it makes sense to explicitly enable it.
sirreal
requested review from
ellatrix,
fluiddot,
dcalhoun,
ajitbohra,
ntwb,
nerrad,
ryanwelcher,
kevin940726,
luisherranz,
DAreRodz,
getdave,
tellthemachines,
artemiomorales and
michalczaplinski
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
sirreal
added
[Type] Code Quality
|
It's a good sign that the autofix worked well that the compiled size changed 0 bytes: #61204 (comment) |
|
I've updated the description with additional details. Notably, this is clearly called out in the Core JavaScript Coding Standards:
|
swissspidy
approved these changes
Apr 30, 2024
|
Nice one, I didn't realize this was allowed. The change makes sense to me. |
sirreal
force-pushed
the
fix/eslint-prettier-and-curly-rule
branch
from
4f2a196
to
555b46c
Compare
The `curly` eslint rule was disabled when using prettier. Some configurations of the rule may conflict with prettier's autoformatting, but the `all` option used in @wordpress/eslint-plugin does not. This aligns with the WordPress JavaScript Standards: > `if`, `else`, `for`, `while`, and `try` blocks should always use > braces, and always go on multiple lines. - WordPress JavaScript Standards: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#blocks-and-curly-braces - `curly` rule description: https://eslint.org/docs/latest/rules/curly#rule-details Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: swissspidy <[email protected]> Co-authored-by: youknowriad <[email protected]>
sirreal
force-pushed
the
fix/eslint-prettier-and-curly-rule
branch
from
555b46c
to
f6c42d6
Compare
|
Adding the autofix commit to git ignored revs in #61253. |
Merged
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Restore the
curlyeslint rule to the recommended eslint config when using prettier.Note to reviewers:
This contains an autofix applied by eslint. It's good to review a sample of the changes although they're likely very safe since this is a core eslint rule.
I recommend reviewing this by commit. The manual changes are separated from the automated changes and can be reviewed in their own commit.
Why?
This is aligned with the Core JavaScript Coding Standards on "blocks and curly braces":
As far as I can tell, this was unintentionally removed when the prettier config was added or in a subsequent upgrade of the eslint-plugin-prettier package. This rule is explicitly declared and I believe it was always intended to be included:
gutenberg/packages/eslint-plugin/configs/es5.js
Line 16 in 5b154e5
This was proposed and abandoned previously in #26886.
How?
See the code.
The prettier config disables this rule, so when we merge in prettier config the rule was turned off.
But the motivation is only for certain configurations we do not use:
"multi-line"or"multi-or-nest". With"all"there should be no conflict with prettier.This restores the rule explicitly when the prettier configuration is added.
This also applies the provided autofix to the entire repo.
Testing Instructions
Code review, manual testing isn't necessary.