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

Use prettier v2 #2324

Merged
merged 9 commits into from Jan 25, 2021
Merged

Use prettier v2 #2324

merged 9 commits into from Jan 25, 2021

Conversation

mroderick
Copy link
Member

@mroderick mroderick commented Jan 3, 2021

This PR upgrades prettier to latest and removes local configuration, adopting prettier default options.

Solution - optional

  • Upgrade prettier
  • Remove .prettierrc, accepting the default options
  • Re-format all JavaScript files using prettier
  • Configure editors via .editorconfig

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm run prettier:check
  4. npm run lint
  5. npm run test-cloud
  6. Observe that there are no warnings or errors

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@mroderick mroderick added the semver:major changes will cause a new major version label Jan 3, 2021
@mroderick mroderick requested a review from mantoni January 3, 2021 13:09
Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

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

👍

@mroderick
Copy link
Member Author

I'd like to merge #2251 before this one, so IE users can have one final fix, before a new major release that drops support for that runtime.

@mroderick mroderick force-pushed the use-prettier-v2 branch 4 times, most recently from 45a321b to 987a9e6 Compare January 25, 2021 16:51
@mroderick mroderick added semver:patch changes will cause a new patch version and removed semver:major changes will cause a new major version labels Jan 25, 2021
@mroderick
Copy link
Member Author

Working on this PR with @hexeberlin today, we discovered that my assumptions about the Prettier upgrade were flawed.

At https://prettier.io/docs/en/options.html#trailing-commas, it is specified that Default value changed from none to es5 in v2.0.0

As in the MDN article about trailing commas, it's clear that ES5 trailing commas are supported in IE11

Therefore, upgrading to prettier@v2 doesn't need to trigger a MAJOR revision, as ES5 trailing commas are supported in the runtimes that we currently support.

This can be verified by checking out this branch and running npm run test-cloud, which will pass in IE11 and Safari 9.

I've updated the label for this PR, to use semver:patch.

This means that we can leave the dropping of IE11 to when we import an upgraded eslint-config-sinon, that allows for more modern syntax, which actually breaks in legacy runtimes.

Every day is a school day...

@mroderick
Copy link
Member Author

I still haven't figured out how to get prettier --check to show what's actually wrong, leading to a poorer development experience than when using eslint-plugin-prettier

@mantoni
Copy link
Member

mantoni commented Jan 25, 2021

It doesn't matter what is wrong. If a file fails the check, it needs to be formatted with prettier. Then it will pass the check. The whole point is that you don't make formatting changes manually anymore.

@mroderick
Copy link
Member Author

It doesn't matter what is wrong. If a file fails the check, it needs to be formatted with prettier. Then it will pass the check. The whole point is that you don't make formatting changes manually anymore.

That pretty much destroys staging parts of files, which I do all the time, to be able to craft a good git narrative

@mroderick mroderick merged commit a5eb1f0 into sinonjs:master Jan 25, 2021
@mroderick mroderick deleted the use-prettier-v2 branch January 25, 2021 23:05
@mantoni
Copy link
Member

mantoni commented Jan 26, 2021

That pretty much destroys staging parts of files

Use a prettier plugin. There is one for every editor. I configured mine to detect prettier configs to have it enabled only for projects that have one. It then formats on save.

@mroderick mroderick mentioned this pull request Jan 26, 2021
mroderick added a commit to mroderick/eslint-config-sinon that referenced this pull request Feb 23, 2021
Downstream projects have switched to using `prettier --check` to verify
that code follows style guide.

Example: sinonjs/sinon#2324
mroderick added a commit to mroderick/eslint-config-sinon that referenced this pull request Feb 23, 2021
Downstream projects have switched to using `prettier --check` to verify
that code follows style guide.

Example: sinonjs/sinon#2324
mroderick added a commit to mroderick/eslint-config-sinon that referenced this pull request Feb 23, 2021
Downstream projects have switched to using `prettier --check` to verify
that code follows style guide.

Example: sinonjs/sinon#2324
mroderick added a commit to mroderick/eslint-config-sinon that referenced this pull request Mar 12, 2021
Downstream projects have switched to using `prettier --check` to verify
that code follows style guide.

Example: sinonjs/sinon#2324
mroderick added a commit to sinonjs/eslint-config that referenced this pull request Mar 12, 2021
Downstream projects have switched to using `prettier --check` to verify
that code follows style guide.

Example: sinonjs/sinon#2324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch changes will cause a new patch version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants