Conversation
Builds ready [5d223ea]
Page Load Metrics (576 ± 45 ms)
|
|
@whymarrh this was opened while you were out, I got some initial responses in slack about some of the changes -- would love to hear your thoughts if you get time between fires/tasks. |
|
We should revisit this once #8982 is completed! |
|
@Gudahtt @whymarrh @rekmarks this has been updated... I actually did the work in a new branch off master then cherry picked the commit into this branch so it's worth looking again at changes. In addition to the rule changes mentioned in the original message, i had to also disable |
Builds ready [23e5166]
Page Load Metrics (449 ± 65 ms)
|
|
@brad-decker I unfortunately created a bunch of merge conflict for you with #9290. Overall, I think this is a significant improvement. My one serious quibble is the addition of lines after comments and the removal of lines before comments. That feels backwards to me. Is that behavior not configurable? |
|
@rekmarks i'll play with it a little more to see if I can finagle it. |
|
@rekmarks so did some further digging.. the only place there is an actual issue with our default setting for lines-around-comments is here:
Prettier doesn't like a space after an object block declaration, it has nothing to do with the comment itself it just happens to conflict with the lines-around-comment rule. I don't think we really want a space before this comment in this case, so I think I have finagled the eslint rules now to very closely match our intention without interfering with prettiers automatic optimization of line breaks. I'll push up the new diff soon. |
|
Alright. |
|
nit: some of the files in |
Builds ready [9476507]
Page Load Metrics (406 ± 56 ms)
|
| const directionTargetGenerator = ({ top, left, height, width }) => { | ||
| const horizontalMiddle = left + (width / 2) | ||
| const verticalMiddle = top + (height / 2) | ||
| const horizontalMiddle = left + width / 2 | ||
| const verticalMiddle = top + height / 2 | ||
| return { | ||
| up: { x: horizontalMiddle, y: top - height }, | ||
| down: { x: horizontalMiddle, y: top + (height * 2) }, | ||
| down: { x: horizontalMiddle, y: top + height * 2 }, | ||
| left: { x: left - width, y: verticalMiddle }, | ||
| right: { x: left + (width * 2), y: verticalMiddle }, | ||
| right: { x: left + width * 2, y: verticalMiddle }, | ||
| middle: { x: horizontalMiddle, y: verticalMiddle }, | ||
| } |
There was a problem hiding this comment.
Commenting to bookmark multiplication/division parenthesis example.
|
I've created a tracking issue for v4 of our ESLint config: MetaMask/eslint-config#67 We should be able to put most of the config changes into that so that v4 allows us to standardize prettier across all of the repos. |
|
I remember seeing a PR made to prettier itself—are we waiting on that to update this? (No rush, just tryna understand.) |
|
@whymarrh I can champion the PR and try and get some people to review it but ultimately when or if that merges is out of our control. My personal, selfish, preference would be to accept the unfortunate change to no-mixed-operators as bookmarked by @rekmarks for now until such a time as I am able to get that PR over the finish line. I don't have a preference for order of operations here in term of what gets merged where first, just want to make sure I'm doing everything I need to do to progress this PR the way the team feels is the most comfortable. |
|
Also, for reference, this is the PR: prettier/prettier#9026 |
|
🎉 rebased. |
Builds ready [5bdc5ec]
Page Load Metrics (388 ± 50 ms)
|
|
🎉 and again. I can do this all day, folks. 💻 ☕ |
Builds ready [30d94db]
Page Load Metrics (563 ± 114 ms)
|
Builds ready [55ad3eb]
Page Load Metrics (524 ± 122 ms)
|
|
Config looks good to me! |
.eslintrc.js
Outdated
There was a problem hiding this comment.
Was there an explanation somewhere for why these three rules were disabled? 🤔
.prettierrc.yml
Outdated
There was a problem hiding this comment.
We should enable semicolons here or in a follow-up. Our TypeScript eslint config demands it.
There was a problem hiding this comment.
follow up, please 😅
Builds ready [eadbf30]
Page Load Metrics (390 ± 70 ms)
|
b3b9b0f to
5cc0bff
Compare
rekmarks
left a comment
There was a problem hiding this comment.
Let's get this merged before anything else happens!
Builds ready [843eebd]
Page Load Metrics (411 ± 65 ms)
|
rationale
Eslint helps to make sure that everyone follows convention, but it doesn't take an opinionated approach to do so. Prettier has some stringent rules that they are rather inflexible on, but in my experience, it results in code formatting. that is universal and enforced on save. It, by default, attempts to keep code fitting within the average width of a half screen code window (80 col) but also maximizes the amount of code per line with the legibility as its primary concern.
The following PR has been split up into commits to make the diff manageable.Looking for feedback on how we can modify this setup to meet the needs of the entire team.Link to
.prettierrc.ymlin feature branch: https://github.com/MetaMask/metamask-extension/blob/prettier/.prettierrc.yml@rekmarks pointed out in slack the changes to the eslint config, copy pasted:
Disabled/attenuated:
indent
no-mixed-operators
no-confusing-arrow
react/jsx-closing-tag-location
react/jsx-wrap-multilines
Added:
react/self-closing-comp
NEW RULE UPDATES AFTER REBASE
disabled: wrap-regex
attenuated: lines-around-comment (prettier has some rules around this that conflict with the default eslint rule config)
Strategy for merging
Once we all agree on the changes and the settings, I can split this into multiple PRs using the following strategy:
For the first PR add prettier, eslint-prettier plugin, and update rules, use overrides by folder path to disable the prettier rules, and revert the changes to the eslint rules that were required to the code. So this should result in only .eslintignore, .eslintrc, package.json and yarn.lock changing.
I can then have successive PRs for each subpath of the application so that the changes can be reviewed in chunks and if we find anything I can pause the progress of merging PRs, fix the eslint config and update any issues resulting from that in previously merged work.
The final PR would remove the overrides altogether.
I think there would be additional steps to lift this configuration to the eslint-config package and distribute prettier with it, then we'd need to gradually adopt it in all of the other repositories. If that's a thing we want to do after using it in extension codebase.