More intuitive same-line placement for list-like values - #52
FuegoFro wants to merge 1 commit into
Conversation
Sorry, something went wrong.
This diff tries to add more intuitive/reasonable placement for values in certain situations. In particular, if we have list-like literals that are too long to fit on a line, we often want the open/close brackets of the literal to be on the same line as the surrounding structure, rather than be entirely on their own line. An example is:
```
foo([
1,
2,
...
10,
])
```
The naive way of using `group`s would cause the `[` and `]` to each be on their own lines, due to how `group` splits from the outside in. This adds optimizations/special cases for this style of formatting in `Assign`s (and the closely related `AnnAssign`s and `AugAssign`s), `Call`s, `Return`s and `Yield`s.
There are fairly extensive tests for the new behavior, but let me know if there's any other cases you'd like to see! In particular there's tests for non-list-like expressions to make sure that "normal" wrapping works as indended, and tests for the various kinds of assignments. There's still some work for how we handle complex (multi-part) strings, but I think that's mostly unrelated to this diff.
I also tried to comment it so it'd be easier to read through in the future, but let me know if there's any part that is confusing or could have better comments!
|
I've been super busy the last couple of weeks, sorry! I'll try and find some time to review this properly soon, but I'd just offer a word of caution with conditionalGroup: it can be quite computationally expensive as it basically causes exponential branches in the doc printer. |
Sorry, something went wrong.
|
No rush! Regarding conditionalGroup, I'd love any suggestions for simpler alternatives of how to implement this behavior! I got the ideas for this from looking at the implementation of function call formatting in the js formatter in the main Prettier repo. From my understanding of how group works, it doesn't seem possible to get this behavior any other way, but I'd love to hear any ideas you have! And regarding the exponential time complexity, my understanding from reading doc-printer.js is that this has basically the same properties as normal group. In particular, my understanding of the algorithm is:
Additionally, it doesn't look like the fits function does any backtracking or breaking of groups (or looking at subsequent expanded states of groups). Given this, I don't think nested conditionalGroups actually are exponential? But I could definitely be wrong! |
Sorry, something went wrong.
|
@azz if you have some time, it'd be awesome if you could take a glance over some of this! I think the most relevant parts to look at would be the printExpandingValue and printMultiAssignment functions :) |
Sorry, something went wrong.
azz
left a comment
There was a problem hiding this comment.
So sorry I hadn't review this yet. I've been busy at work and moving house, so my OSS hours have been limited.
Everything here LGTM. Great job!
Sorry, something went wrong.
|
Also r.e. complexity, I have never actually done the analysis on the code, I think it was @vjeux who mentioned that it was exponential? I seem to recall performance issues in Prettier JS due to it but I could be mistaken. |
Sorry, something went wrong.
vjeux
left a comment
There was a problem hiding this comment.
Yeah, conditionalGroup is exponential in terms of runtime cost if you nest them. If you're doing it on every single values that can be deeply nested, then you're going to have a bad time.
This is something that plagues most pretty printers and prettier algorithm (outside of conditionalGroup which was added as a hack) has been designed to avoid this. If you don't want the next year of your life to fight performance issues, I would highly recommend you to avoid it like the plague.
Sorry, something went wrong.
| long_item_1_aaaaaaaaaaaa, | ||
| long_item_1_aaaaaaaaaaaa, | ||
| long_item_1_aaaaaaaaaaaa | ||
| ]), |
There was a problem hiding this comment.
It seems like your goal is to not have [] be on its own line. You don't need conditionalGroup for this. What you can do when you are printing a function call is to look at the type of the arguments and if there's a single argument which is an array, then output something different. The whole game of prettier is from the ast to figure out what IR to output such that you get the right thing at the end.
Sorry, something went wrong.
| my_long_variable_1_aaaaaaaaaaaaaaaaaaaaaa = \\ | ||
| my_long_variable_2_aaaaaaaaaaaaaaaaaaaaaa = \\ | ||
| my_long_variable_3_aaaaaaaaaaaaaaaaaaaaaa = \\ | ||
| my_long_value_aaaaaaaaaaaaaaaaaaaaaaaaaa |
There was a problem hiding this comment.
You should be able to get the result that you want without conditional groups using carefully crafted groups, indents and ifBreak.
Sorry, something went wrong.
|
Sorry for leaving this open for so long! I haven't had time to work on this in a while, so I'm going to go ahead and close this out. If people still feel this is valuable, we can take another stab at addressing some of the complexity concerns. For what it's worth, I still am not convinced this is actually any slower or more asymptotically complex than normal group, but again that discussion could happen at a later time. |
Sorry, something went wrong.
This diff tries to add more intuitive/reasonable placement for values in certain situations. In particular, if we have list-like literals that are too long to fit on a line, we often want the open/close brackets of the literal to be on the same line as the surrounding structure, rather than be entirely on their own line. An example is:
foo([ 1, 2, ... 10, ])The naive way of using groups would cause the [ and ] to each be on their own lines, due to how group splits from the outside in. This adds optimizations/special cases for this style of formatting in Assigns (and the closely related AnnAssigns and AugAssigns), Calls, Returns and Yields.
There are fairly extensive tests for the new behavior, but let me know if there's any other cases you'd like to see! In particular there's tests for non-list-like expressions to make sure that "normal" wrapping works as indended, and tests for the various kinds of assignments. There's still some work for how we handle complex (multi-part) strings, but I think that's mostly unrelated to this diff.
I also tried to comment it so it'd be easier to read through in the future, but let me know if there's any part that is confusing or could have better comments!