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

Deprecate jsxBracketSameLine option in favour of multi-language bracketSameLine option #11006

Merged
merged 22 commits into from Aug 31, 2021

Conversation

kurtztech
Copy link
Contributor

@kurtztech kurtztech commented Jun 3, 2021

Description

I've added a new option bracketSameLine that was approved to address issue #5377, essentially making a generic version of the jsxBracketSameLine option that works for HTML, JSX, Vue, and Angular. I've deprecated jsxBracketSameLine. It still works, but it is now listed as deprecated in the docs and CLI, and has been removed from the playground options.

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

We should keep old options (include CLI) for compatibility, just log deprecation

@kurtztech kurtztech changed the title poc: replace jsxBracketSameLine option with angleBracketSameLine option Replace jsxBracketSameLine option with angleBracketSameLine option Jun 4, 2021
@kurtztech kurtztech changed the title Replace jsxBracketSameLine option with angleBracketSameLine option Deprecate jsxBracketSameLine option in favour of multi-language angleBracketSameLine option Jun 4, 2021
@kurtztech kurtztech marked this pull request as ready for review June 4, 2021 02:16
@kurtztech kurtztech changed the title Deprecate jsxBracketSameLine option in favour of multi-language angleBracketSameLine option Deprecate jsxBracketSameLine option in favour of multi-language bracketSameLine option Jun 4, 2021
@kurtztech kurtztech requested a review from fisker June 8, 2021 17:50
@kurtztech
Copy link
Contributor Author

@fisker is there anything else I need to do before this PR can be accepted?

@fisker
Copy link
Sponsor Member

fisker commented Jun 11, 2021

I reverted unnecessary changes, and wrote some new tests.

changelog_unreleased/html/11006.md Outdated Show resolved Hide resolved
src/language-js/options.js Outdated Show resolved Hide resolved
@kurtztech kurtztech requested a review from fisker June 16, 2021 16:17
@fisker
Copy link
Sponsor Member

fisker commented Jun 17, 2021

Can you fix tests?

@kurtztech
Copy link
Contributor Author

kurtztech commented Jun 18, 2021

@fisker tests are fixed, I ended up just having to remove the default property from the jsxBracketSameLine option after deprecation.

Copy link
Sponsor Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

LGTM.

One comment about test.

I'm not good at docs, we'll need other maintainers to approve.

description: "Put > on the last line instead of at a new line.",
deprecated: "2.4.0",
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We had test for deprecated options before, can we add one for this? Check https://github.com/prettier/prettier/pull/7536/files, the old tests_integration/__tests__/arg-parsing.js file is moved to tests/integration/__tests__/arg-parsing.js

@kianmeng-aon
Copy link

exactly what i was looking for! would love to use this feature, looking forward for the release

kudos kurtztech ✨

@xianshenglu
Copy link

xianshenglu commented Jul 9, 2021

Looking forward for the release. May I asked about the possible release time?

@gethari
Copy link

gethari commented Jul 13, 2021

Waiting for this !

@sosukesuzuki sosukesuzuki self-requested a review July 13, 2021 12:28
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

This PR should not include updating markdown files under /website/versioned_docs/version-stable/. It should be updated when we release 2.4.0.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's fix problems above and I think we can merge it

@kurtztech
Copy link
Contributor Author

Let's fix problems above and I think we can merge it

So is it just the markdown file changes I need to remove, or are there other issues?

@sosukesuzuki
Copy link
Member

sosukesuzuki commented Jul 19, 2021

@kurtztech Please check a comment from @fisker. You need to add more tests for deprecated options. (#11006 (comment))

@sosukesuzuki sosukesuzuki merged commit 4992d97 into prettier:main Aug 31, 2021
@HADB
Copy link

HADB commented Aug 31, 2021

Great job! 👍🏻

@glen-84
Copy link

glen-84 commented Sep 9, 2021

The option name is super generic ... #5377 (comment).

Too late to change that now I suppose.

Alhadis added a commit to Alhadis/JG that referenced this pull request Nov 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet