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

chore(deps): reject auto upgrade of prettier #2843

Closed
wants to merge 1 commit into from

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented May 17, 2021

prettier@2.3.0 is now enforcing Allman brace styles, while the code
base uses 1tbs (one true brace style).

In fact, the latter has been explicitly configured in eslint.
There does not seem to be a way to configure prettier to switch styles.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

prettier@2.3.0 is now enforcing Allman brace styles, while the code
base uses 1tbs (one true brace style).

In fact, the latter has been explicitly configured in eslint.
There does not seem to be a way to configure prettier to switch styles.
@nija-at nija-at requested a review from a team May 17, 2021 15:32
@nija-at nija-at self-assigned this May 17, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 17, 2021
@mergify
Copy link
Contributor

mergify bot commented May 17, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 17, 2021
@mergify
Copy link
Contributor

mergify bot commented May 17, 2021

Merging (with squash)...

@eladb
Copy link
Contributor

eladb commented May 17, 2021

Isn't it possible to configure prettier accordingly?

@MrArnoldPalmer
Copy link
Contributor

It looks related to this change which only applies to classes with multiline extends/inherits https://prettier.io/blog/2021/05/09/2.3.0.html

We likely could make eslint ignore this and just let prettier handle it (thats what eslint-config-prettier is for but maybe it hasn't caught up?).

At the top of that same release notes page it recommends not using a ^ version of prettier though so this seems good anyway.

@nija-at nija-at added pr/do-not-merge This PR should not be merged at this time. and removed pr/ready-to-merge This PR is ready to be merged. labels May 18, 2021
@nija-at
Copy link
Contributor Author

nija-at commented May 18, 2021

Isn't it possible to configure prettier accordingly?

@eladb - I could not find such an option. I believe this is because of prettier's philosophy

We likely could make eslint ignore this and just let prettier handle it

@MrArnoldPalmer - Here's how that would look - #2835. Is that a better change?

@nija-at nija-at requested review from eladb and MrArnoldPalmer May 18, 2021 08:45
@MrArnoldPalmer
Copy link
Contributor

@nija-at yeah lets take that. I think just letting Prettier handle everything it does and using Eslint only for non-formatting related stuff is our preferred experience. TY for that!

@nija-at nija-at closed this May 18, 2021
@nija-at nija-at deleted the nija-at/lock-prettier branch May 18, 2021 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants