-
Notifications
You must be signed in to change notification settings - Fork 469
Smart linebreak printer for pipe chains #6380
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
Comments
This would be great indeed. |
Who wants to work on exploring this? Any volunteers? We'll provide the coaching needed of course. |
Thanks for the PR, but telling from the tests that are changed it is probably not "smart"? Smart printing means that it will be preserved on one line iff the user wrote it on a single line (except of course it is above the max length). Otherwise it should be multiline. Essentially it would be the same behaviour as with records or variants. |
Personally, I don't like the concept of smart formatting. I think it's good that the code is always formatted the same way. |
It's a huge bikeshedding topic and ppl. complained about being forced to have their records multiline before so ofc it is a compromise. The formatter is not perfect and finding good enough heuristics that work every time without making your code inconsistent is very hard. Alternatively, there is also the suggestion from TheSpyder in the old thread to just break when you hit the third pipe (rescript-lang/syntax#257 (comment)). That won't produce as many diffs I think. |
I like the suggestion. But still should remember about the line length. So if you have a very nested function call with two pipes, I think it also needs to be split into multiple lines. |
Yes, but that is already the case so I did not mention it explicitly. |
I think we should break when it exceeds 80 columns and not three pipes. Also adding a space around the pipe improves readability. Yes that's not "smart", I just copied the title. (*example ocamlformat*)
let baz = [1; 2; 3] |> List.map (fun x -> x + 1) |> List.map (fun x -> x + 1) |
I think that's more relevant to OCaml, than ReScript. |
This is how prettier works 👍 Also, we might have both 80 columns and three pipes. |
Why? Some languages that use pipe add a space around it (elixir, R, julia). I always thought the glued pipe operators were weird. |
But not Js (if we consider the method call as the closest relative to pipes) |
They are glued to resemble something like PHP-style method calls. It triggers autocomplete, so I think having to write 3 or 4 chars instead of 2 is worse. As for the 80 chars to break, why would we want to break anything below the maximum line width of 100? I just realised there may be value in breaking all of them if we want this. However, smart printing would be just as feasible. As you can see, it definitely is a huge bikeshedding topic... 😅 |
Note that PR breaks pipe when there are three pipes. So the code below is not formatted let bazz = foo.bars->OptionEx.arrayValues But let bazz = foo.bars->OptionEx.arrayValues->Map.String.fromArray Is formmated to: let bazz =
foo.bars
->OptionEx.arrayValues
->Map.String.fromArray |
@fhammerschmidt what are we choosing between here? 👀 I don't fully follow the discussion. |
|
I'm not familiar enough with the parser/printer. Does anyone have any sort of assessment of how much work/how difficult option 3 would be, smart breaking? My immediate feeling that something like how records work today would be the best way. As in remember the formatting up until a point (3 pipes? 2 pipes?), then just force format as line breaks. Maybe this just rehashes what you've all already said. |
In fact, there are two pipes connecting three expressions.I think this should be the proposal. |
I dipped my toes in the printer and came up with a (hopefully) smart enough solution. |
@fhammerschmidt, so your implementation will only take effect if I write it on another line? This won't be formatted, right? let bazz = foo.bars->OptionEx.arrayValues->Map.String.fromArray
|
Correct. Your example would stay as-is. |
This is an old issue from the syntax repo. Would be still nice to have.
amiralies commented on Feb 4, 2021
Ref: rescript-lang/syntax#257
The text was updated successfully, but these errors were encountered: