Upgrade Prettier - #51065
Conversation
levadadenys
left a comment
levadadenys
left a comment
There was a problem hiding this comment.
Wow, well done! Like those rules improvements! LGTM!
(As for opted out changes):
+1 for trailing commas in the end of objects.
And just of curiosity, what's the pros of Always wrap arrow function arguments with parentheses. I.e. go from const fn = x => … to const fn = (x) => …. ? (IMO const fn = x => … is a little bit cleaner and more common)
|
My first job had the trailing comma rule, and it was an absolute pleasure. I would love to do this in our repo. But I do think if we enable it, doing it as a separate PR makes sense –– there are already so many files changed here. I'm also pro keeping the parens for I was visually surprised by the In short, I'm a fan of all the defaults! Thanks for your work on this. As far as test coverage, I agree with your statement
In that case, thoughts about disabling this rule for now and enabling it in a separate PR? It'd be easier to (a) revert if needed, and (b) get eyes on exactly what is changing. |
|
@megcrenshaw I agree we could discuss the other defaults and potentially do them as follow-ups. I feel ambivalent about the trailing commas, and lean towards the parens around single params. For the quotes/numbers, I found all the cases where that was changed and either validated them locally or added an ignore if I could not validate. Do you think that's sufficient? Unfortunately the prettier configuration value we would apply in this case is preserve, which would mean I would either need to manually track down the quote changes (and hope I didn't miss any), or start over. |
Ah, I see re: |
sanchitmalhotra126
left a comment
sanchitmalhotra126
left a comment
There was a problem hiding this comment.
LGTM! I'm good with the defaults you've chosen.
I've haven't used the trailing comma rule (and initially thought I'd be opposed to it), but would be curious to try it out if it makes for cleaner diffs! FWIW, I personally use a prettier plugin in VSCode to lint as I go, so I don't think i'd notice either way. |
|
@levadadenys here is their reasoning for the parens around arrow params. |
Oh, I see now, thanks for sharing. Agree, sounds reasonable to me. It can make dev process slightly more comfortable |
4 participants
Upgrade to the latest version of Prettier (2.8.7). This is necessary so that we can add typescript linting to our repo. The main changes applied here are:
I opted out of the following default changes in this upgrade (see
.prettierrc.js)Links
Testing story
I am relying on drone to validate this change. I don't see any issues with the spacing changes. I spot tested the number changes and saw no issues, although I added one ignore here because it was applying the rule inconsistently.
If you want to view the changes, I would highly recommend viewing with whitespace hidden. I looked at all the non-test file changes with whitespace hidden and it took me quite a while. I spot checked some of the test file changes, but since they are all run by drone they felt less necessary to validate manually.
PR Checklist: