Format source code with current gren format #11
Conversation
There was a problem hiding this comment.
This is super promising! I've got a few comments, but only the comments regarding the code block with await function calls is what I consider important enough for 0.2.
Fantastic work on this, @avh4 !
| Program.await Node.initialize | ||
| <| (\nodeConfig -> | ||
| Program.await FileSystem.initialize | ||
| <| (\fsPermission -> | ||
| Program.startProgram | ||
| { model = | ||
| { stdout = nodeConfig.stdout | ||
| } | ||
| , command = | ||
| case nodeConfig.args of | ||
| [ _, _, file ] -> | ||
| FileSystem.openForRead fsPermission file | ||
| |> Task.attempt OpenResult | ||
|
|
||
| _ -> | ||
| Stream.send nodeConfig.stderr | ||
| <| BE.encode | ||
| <| BE.string "Exactly one argument is required: the file name to read\n" | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I hope we can format this block of code in a nicer way.
<|seems to always be split out to a seperate line, I'd usually prefer it to be on the same line. I believe that's what happens inelm-formattoo, or am I wrong?- As with
<|, I prefer lambdas to stay on the previous line, instead of being broken out to a separate line. Not sure whatelm-formatdoes in this case. - Wrapping a lambda in parenthesis should be uneccessary when on the right side of
<|.
While I'm giving this feedback to this particular block of code, my comments apply universally.
Personally, I'd love to see the await statements formatted to be on the same line, but I appriciate there being different views on this and it's not important to get in before the 0.2 release.
There was a problem hiding this comment.
👍 Sounds good. I already was expecting this from following some of the discussions on Zulip :D I think this will be doable, but as it's quite different from elm-format, it'll take a bit of thinking through the details of the rules, so I've postponed worrying about it yet.
(1): Yes, elm-format will keep it on a single line if it's not otherwise forced to expand. For gren format, retaining information about where linebreaks originally were isn't implemented at all yet, so allowing single-line <| would fit as part of that future goal. I guess the main question here is, is <| going to be considered special, or do we want a rule that applies to all binary operators? (For elm-format, <| is in fact a special case, see below.)
(2): elm-format doesn't allow that, though it does have one special case for <| \_ -> ... where the <| will stick at the end of the previous line (but the lambda will still start on the next line). This sounds fine to try to implement.
(3): I think I made it add parens in the first pass because I wasn't confident that I understood the edge cases well enough to be confident that it wouldn't remove parens in some cases that are actually necessary. (Since the parser originally dropped all parens when parsing, the formatter actually is only adding parens that are required -- which in total has the result of removing unnecessary parens.) This should be fine to implement. Though if anyone happens to have some time to help enumerate all the possible edge cases where parens may or may not be required around lambdas to avoid changing the meaning of code, that'd be helpful!
| Html.div | ||
| [] |
There was a problem hiding this comment.
When the first argument is a simple expression, like [] or an Int or a String, I think it looks nicer when being placed on the same line as the function call. I believe that's also how elm-format works.
There was a problem hiding this comment.
Yes, elm-format allows function calls to be one of (with the added rule that if any argument spans multiple lines, then it is not allowed to be on the same line as anything else):
- function and all arguments on the same line
- function and first argument on the same line, all other arguments on individual lines
- everything on separate lines
It's a rule that I feel like isn't very elegant, and I guess it's been fine in Elm, but I'm not 100% sure it always makes sense.
We could use that same rule for gren, or consider some different kind of rule for this.
| [ Html.text | ||
| <| String.fromInt model | ||
| ] |
There was a problem hiding this comment.
Here, I think it makes sense to format as Html.text <| String.fromInt model.
There was a problem hiding this comment.
👍 this is part of retaining linebreak information, noted in #11 (comment) re "(1)"
| bookButton : | ||
| { isDisabled : Bool | ||
| } | ||
| -> Html Msg |
There was a problem hiding this comment.
IMHO, type signatures containing records with at most one property, looks nicer when formatted on a single line.
There was a problem hiding this comment.
👍 Yeah, I was thinking it would be a good rule for arrays and records (for types, and expressions, and maybe also patterns?) that if there's only a single element, prefer single-line formatting.
| ++ String.fromInt date.month | ||
| ++ "." | ||
| ++ String.fromInt date.year | ||
| String.fromInt date.day ++ "." ++ String.fromInt date.month ++ "." ++ String.fromInt date.year |
There was a problem hiding this comment.
I guess this one is tricky.
In this concrete case, I'd rather this being broken up in separate lines. But simple cases such as (a ++ b) looks better in a single line.
There was a problem hiding this comment.
Similar to single-element arrays and records, I could try preferring single-line if there's only one operator in the sequence.
| { celsius = "" | ||
| , fahrenheit = "" | ||
| } | ||
|
|
||
|
|
||
| --- UPDATE | ||
|
|
||
| -- - UPDATE |
There was a problem hiding this comment.
This is interesting. How does the space get in there?
There was a problem hiding this comment.
Ah, it tries to insert a leading space in the comment if there isn't one, but I guess that doesn't make sense if it starts with -! I'll have to check how I did it in elm-format.
| import Html exposing ( Html, text, div, input, button, select, option ) | ||
| import Html.Attributes exposing ( id, style, value, disabled ) |
There was a problem hiding this comment.
Should the imports be automatically sorted?
| import Html exposing ( Html, text, div, input, button, select, option ) | |
| import Html.Attributes exposing ( id, style, value, disabled ) | |
| import Html exposing ( Html, button, div, input, option, select, text) | |
| import Html.Attributes exposing ( disabled, id, style, value ) |
This formats the examples with
gren formatfrom gren-lang/compiler@256eeaa which is still WIP, but is at a point where it will safely format this repo (all code is unchanged, and all comments are retained).Feel free to merge if you like, or use this as a place to comment on anything you'd like to see changed in the formatter before applying it to the official code repos.