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 ternary format in function call #4368

Merged
merged 6 commits into from Aug 15, 2018

Conversation

Projects
None yet
7 participants
@malcolmsgroves
Contributor

malcolmsgroves commented Apr 25, 2018

This fixes #4338. The formatting of conditional expression in functional calls was hard to read. This PR results in the following format:

fn(
  bifornCringerMoshedPerplexSawder,
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie === averredBathersBoxroomBuggyNurl &&
    anodyneCondosMalateOverateRetinol
    ? annularCooeedSplicesWalksWayWay
    : kochabCooieGameOnOboleUnweave
);

While it would be preferable to indent the last two operands of the ternary expression, it seemed to me like that would require substantial refactoring of the printer because the last two operands are not in the scope of the test operand and therefore do not inherit the indentation level. At least this makes it easier to see how many args there are.

This is my first Prettier PR, so apologies for any oversights. Happy to try to get those last two lines to indent if anyone has any ideas for how to proceed.

malcolmsgroves added some commits Apr 24, 2018

@suchipi

This comment has been minimized.

Member

suchipi commented May 2, 2018

I think this is only halfway there; we need to indent the last two lines in this case, too (as you noted). I don't think we should merge a change until it does that as well.

@malcolmsgroves

This comment has been minimized.

Contributor

malcolmsgroves commented May 6, 2018

makes sense. I'm stuck on this one right now, so someone else can feel free to take up the issue or provide suggestions for how to proceed

@duailibe duailibe added the status:wip label May 7, 2018

@j-f1

This comment has been minimized.

Member

j-f1 commented Jun 28, 2018

I propose merging this as-is since it’s strictly better than the current behavior, but leaving the issue open to figure out a better solution. How does that sound?

j-f1 added some commits Aug 13, 2018

@suchipi

This comment has been minimized.

Member

suchipi commented Aug 15, 2018

@j-f1 sounds good to me

@lipis

lipis approved these changes Aug 15, 2018

@j-f1

j-f1 approved these changes Aug 15, 2018

@j-f1 j-f1 merged commit d480858 into prettier:master Aug 15, 2018

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 1790211...c2df7c0
Details
codecov/project 96.48% remains the same compared to 1790211
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@j-f1

This comment has been minimized.

Member

j-f1 commented Aug 15, 2018

Thank you for contributing @malcolmsgroves!

@thorn0

This comment has been minimized.

Contributor

thorn0 commented Aug 15, 2018

@j-f1 You were going to keep #4338 open, but GitHub closed it automatically.

@j-f1

This comment has been minimized.

Member

j-f1 commented Aug 15, 2018

Thanks for pointing that out! Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment