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

fix alignTernaryLines behavior #46

Merged
merged 4 commits into from
Feb 20, 2019
Merged

Conversation

aMarCruz
Copy link

See prettier#44

  • 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 not an internal change) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Copy link
Collaborator

@brody4hire brody4hire left a comment

Choose a reason for hiding this comment

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

@aMarCruz I would like to first thank you on behalf of the entire user community for this contribution.

I will probably need a few days for a more careful review.

As I said in the line comments, it would be great if you could explain the motivation for each of the two lines you added to tests/standard/jsfmt.spec.js, One of the added lines is already merged in from PR #47 and should be removed from this proposal (by merge or rebase). I would favor moving the other one to its own PR as well.

@@ -1,4 +1,6 @@
run_spec(__dirname, ["babel", "flow", "typescript"], {
arrowParens: "always",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aMarCruz can you explain the motivation for proposing this change?

I would probably favor making this change in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

ups, I thought that setting was from Standard, but it is from my own config.

@@ -1,4 +1,6 @@
run_spec(__dirname, ["babel", "flow", "typescript"], {
arrowParens: "always",
endOfLine: "lf",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is now merged in from PR #47, should be removed from this proposal by merge or rebase.

@brody4hire brody4hire merged commit f115254 into bangkokjs:master Feb 20, 2019
@brody4hire
Copy link
Collaborator

Thanks @aMarCruz for the awesome work, it is now merged. I will try to release it soon.

@aMarCruz
Copy link
Author

aMarCruz commented Feb 20, 2019

@brodybits , thanks!

As a side note, when using alignTernaryLines:false, each ternary is indented as a block using the user's prferences and avoiding the default 2-spaces indention, much like Prettier's previous behavior.
This is done even in JS fragments inside .jsx and .tsx files, as ESLint does.

This style is not so pretty but it avoids conflicts and is quite consistent.

@brody4hire
Copy link
Collaborator

Thanks @aMarCruz for the side note. Should we consider documenting this, or maybe just adding a comment somewhere?

@aMarCruz
Copy link
Author

@brodybits, sure. But I don't know where and my "english" is poor.

Anyway, in the wiki of eslint-plugin-prettierx I will explain details about the behavior. I hope to have a usable version in one or two days, ...right now I'm $tarting a new project :)

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

Successfully merging this pull request may close these issues.

2 participants