Skip to content

Conversation

suchipi
Copy link
Member

@suchipi suchipi commented Aug 31, 2018

Changes formatting of switch-style nested ternaries to the suggestion in #4767 (comment).

Fixes #737

  • I’ve added tests to confirm my change works.
  • (n/a) (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • I’ve read the contributing guidelines.

? ddddddddddddddd
: eeeeeeeeeeeeeee
? fffffffffffffff
: gggggggggggggggg;
Copy link
Member

Choose a reason for hiding this comment

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

This one seems not working.

Copy link
Collaborator

@duailibe duailibe left a comment

Choose a reason for hiding this comment

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

I like it

? "partially visible"
: "hidden"
? "partially visible"
: "hidden"
Copy link
Member

@ikatyang ikatyang Sep 4, 2018

Choose a reason for hiding this comment

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

This shouldn't be changed, we have a special case for ternary in ternary to use tabs in --use-tabs mode, that's what the removed part in 3018dd1 wanted to achieve.

Edit: I meant indent, not just tabs.

Copy link
Member Author

@suchipi suchipi Sep 4, 2018

Choose a reason for hiding this comment

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

Ok, thanks. I will fix it. I appreciate the insight

@lydell
Copy link
Member

lydell commented Oct 13, 2018

@suchipi Do you think you’ll be able to finish this one up?

@duailibe
Copy link
Collaborator

@ikatyang Can you check if 23cc0cc fixed it?

@ikatyang
Copy link
Member

--use-tabs is working, but spaces are not.

Prettier pr-5039
Playground link

--parser babylon
--print-width 1
--tab-width 5

Input:

a ? 1:2?1?3:4:2

Output:

a
     ? 1
     : 2
     ? 1
       ? 3
       : 4
     : 2;

Expected Output

a
     ? 1
     : 2
     ? 1
          ? 3
          : 4
     : 2;

? "boolean"
: T extends undefined
? "undefined"
: T extends Function ? "function" : "object";
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR: We should force breaking nested TS type ternaries just like normal ternaries.

@suchipi
Copy link
Member Author

suchipi commented Oct 14, 2018

Thanks @ikatyang, my life has been really busy lately because I am getting a new job and my apartment got flooded, I really appreciate the help ❤️

@duailibe duailibe merged commit 0fa34b2 into prettier:master Oct 14, 2018
@duailibe
Copy link
Collaborator

Great work @suchipi and @ikatyang. Thank you!

@davidmccabe
Copy link

davidmccabe commented Nov 5, 2018

This is a huge improvement, thanks for working on it.

Looking at the test file, it seems that one of the examples is still not flattened, lines 5 – 11:

const value = condition1
? value1
: condition2
    ? value2
    : condition3
        ? value3
        : value4;

It's not clear why this example is formatted differently from the others. Is this intended?

@suchipi
Copy link
Member Author

suchipi commented Nov 5, 2018

@davidmccabe could you link the the file and line that you're looking at?

It's possible that you're looking at the unformatted input. Our snapshots look like this:

input
~~~~~~~~~
output

@fbartho
Copy link

fbartho commented Dec 15, 2018

@suchipi / @ikatyang -- It looks like this PR implemented changes that led to this issue being opened: #5257 -- would somebody mind commenting in there to see if that issue should be instead marked as a "Won't-fix"/"Working as intended"?

If this is the new way, and that issue isn't a bug so much as a stylistic preference that doesn't match with the changes introduced here, then I'll just deal.

@lydell
Copy link
Member

lydell commented Dec 15, 2018

@fbartho This PR intended to flatten the “tail” of ternaries – if it caused #5257 it is a bug.

@duailibe
Copy link
Collaborator

duailibe commented Dec 17, 2018

@fbartho that issue was open in Oct 12, this PR was only merged in Oct 14. These are probably unrelated

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Mar 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants