Skip to content
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

Update prettier, change print width to 120, and enable dangling commas for functions #371

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Feb 9, 2024

Fixes #370

I didn't regenerate all workloads (yet). Please do not land this just yet, this PR is here so that we can decide about #370.

I made separate commits so that we can easily see the effect of each action:

  1. update Prettier
  2. add dangling commas to functions
  3. change printWidth to 120

@julienw julienw marked this pull request as draft February 9, 2024 13:09
@rniwa
Copy link
Member

rniwa commented Feb 24, 2024

At this point, I'd rather not take a huge PR like this into the repo right before the release even if it should not have any real impact on performance in theory.

@rniwa rniwa added the v3.1 label Feb 24, 2024
@rniwa
Copy link
Member

rniwa commented Feb 24, 2024

Adding v3.1 label to match the label on #370

@julienw
Copy link
Contributor Author

julienw commented Mar 5, 2024

At this point, I'd rather not take a huge PR like this into the repo right before the release even if it should not have any real impact on performance in theory.

fully agreed, this was definitely meant for after the release. Thanks for adding the label.

@julienw julienw force-pushed the change-print-width branch 2 times, most recently from ec73337 to 231c0d7 Compare March 7, 2024 09:20
Copy link
Contributor

@camillobruni camillobruni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loooking good. I guess we can redo the formatting now that we've released?

@julienw
Copy link
Contributor Author

julienw commented Jun 5, 2024

Loooking good. I guess we can redo the formatting now that we've released?

yeah, I was waiting to see if there was some interest before doing more work :-)

@camillobruni
Copy link
Contributor

I'd definitely prefer some width-limit, especially for the html files.

@julienw julienw force-pushed the change-print-width branch from 231c0d7 to c6b86de Compare June 5, 2024 12:36
@julienw
Copy link
Contributor Author

julienw commented Jun 5, 2024

I rebased and refreshed all affected benchmarks.

On my Linux:

Firefox Before:
image

Firefox After:
image

Chrome Before:
image

Chrome After:
image

(I don't have a Mac so I can't try Safari)

@julienw julienw marked this pull request as ready for review June 12, 2024 17:12
@@ -1,4 +1,4 @@
<!DOCTYPE html>
<!doctype html>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with these DOCTYPE change? These don't seem desirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from the prettier update to v3 (see https://prettier.io/blog/2023/07/05/3.0.0.html#html). There's also no option to disable this behavior.

The doctype is case insensitive so IMO it's not important. I'm glad somebody else has an opinion about it so that I don't need to have one.

@@ -1,12 +1,4 @@
{
"arrowParens": "always",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we making all these other changes to prettier?

Copy link
Contributor Author

@julienw julienw Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find out more if you look at the individual commits, as there's one commit for each logical change.

In short a lot of them are the defaults (see 3rd commit), except printWidth and trailingComma which are the changes by this PR.

Changing trailingComma is beneficial because it makes better diffs: with the old style, there's no trailing comma, and therefore when function parameters or array properties or object properties are added or removed, the previous line also shows up in a diff. If it has a trailing comma, then it isn't changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not removing the default ones would make this PR easier to review/less scary

@julienw
Copy link
Contributor Author

julienw commented Nov 28, 2024

ping @rniwa

@@ -1,12 +1,4 @@
{
"arrowParens": "always",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not removing the default ones would make this PR easier to review/less scary

Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the prettier printWidth option back to the default, or something smaller
4 participants