Skip to content
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.

Sign up for GitHub

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

Format source code with current gren format #11

Open
wants to merge 1 commit into
base: gren-0.2.0
Choose a base branch
from
Open

Format source code with current gren format #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 41 additions & 28 deletions cat/src/Main.gren
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
module Main exposing (main)
module Main exposing ( main )

import Node
import Bytes exposing (Bytes)
import Bytes exposing ( Bytes )
import Bytes.Encode as BE
import Stream exposing (Stream)
import Node.Program as Program exposing (Program)
import FileSystem
import Node
import Node.Program as Program exposing ( Program )
import Stream exposing ( Stream )
import Task


Expand All @@ -19,36 +19,50 @@ main =


type alias Model =
{ stdout : Stream }
{ stdout : Stream
}


type Msg
= OpenResult (Result FileSystem.AccessError (FileSystem.ReadableFileHandle Never))
| ReadResult (Result FileSystem.UnknownFileSystemError Bytes)


init : Program.AppInitTask { model : Model, command : Cmd Msg }
init :
Program.AppInitTask
{ model : Model
, command : Cmd Msg
}
init =
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"
}


update : Msg -> Model -> { model : Model, command : Cmd Msg }
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"
}
)
)
Comment on lines +37 to +57
Copy link
Member

Choose a reason for hiding this comment

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

I hope we can format this block of code in a nicer way.

  1. <| 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 in elm-format too, or am I wrong?
  2. As with <|, I prefer lambdas to stay on the previous line, instead of being broken out to a separate line. Not sure what elm-format does in this case.
  3. 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.

Copy link
Author

Choose a reason for hiding this comment

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

👍 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!



update :
Msg
-> Model
-> { model : Model
, command : Cmd Msg
}
update msg model =
case msg of
OpenResult (Ok fh) ->
Expand All @@ -72,4 +86,3 @@ update msg model =
{ model = model
, command = Cmd.none
}

39 changes: 24 additions & 15 deletions counter/src/Main.gren
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,31 @@
module Main exposing (main)
module Main exposing ( main )

import Browser
import Html exposing (Html)
import Html exposing ( Html )
import Html.Attributes as Attribute
import Html.Events as Event


main =
Browser.sandbox
{ init = init
, update = update
, view = view
}
{ init = init
, update = update
, view = view
}


type alias Model = Int
type alias Model =
Int


init : Model
init =
init =
0


type Msg = Clicked
type Msg
= Clicked


update : Msg -> Model -> Model
update msg model =
Expand All @@ -31,14 +35,19 @@ update msg model =


view : Model -> Html Msg
view model =
Html.div []
[ Html.span
[ Attribute.id "count" ]
[ Html.text <| String.fromInt model ]
view model =
Html.div
[]
Comment on lines +39 to +40
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.span
[ Attribute.id "count"
]
[ Html.text
<| String.fromInt model
]
Comment on lines +44 to +46
Copy link
Member

Choose a reason for hiding this comment

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

Here, I think it makes sense to format as Html.text <| String.fromInt model.

Copy link
Author

Choose a reason for hiding this comment

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

👍 this is part of retaining linebreak information, noted in #11 (comment) re "(1)"

, Html.button
[ Attribute.id "increase-count"
, Event.onClick Clicked
]
[ Html.text "Count" ]
[ Html.text "Count"
]
]
70 changes: 43 additions & 27 deletions files/src/Main.gren
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module Main exposing (main)
module Main exposing ( main )

import Browser
import File exposing (File)
import File exposing ( File )
import Html exposing (..)
import Html.Attributes exposing (..)
import Html.Events exposing (..)
Expand All @@ -13,38 +13,47 @@ import Json.Decode as D


main =
Browser.element
{ init = init
, view = view
, update = update
, subscriptions = subscriptions
}
Browser.element
{ init = init
, view = view
, update = update
, subscriptions = subscriptions
}



-- MODEL


type alias Model = Array File
type alias Model =
Array File


init : {} -> { model : Model, command : Cmd Msg }
init :
{}
-> { model : Model
, command : Cmd Msg
}
init _ =
{ model = [], command = Cmd.none }
{ model = []
, command = Cmd.none
}



-- UPDATE


type Msg
= GotFiles (Array File)
= GotFiles (Array File)


update msg model =
case msg of
GotFiles files ->
{ model = files, command = Cmd.none }
case msg of
GotFiles files ->
{ model = files
, command = Cmd.none
}



Expand All @@ -53,7 +62,7 @@ update msg model =

subscriptions : Model -> Sub Msg
subscriptions model =
Sub.none
Sub.none



Expand All @@ -62,19 +71,26 @@ subscriptions model =

view : Model -> Html Msg
view model =
div []
[ input
[ type_ "file"
, multiple True
, on "change" (D.map GotFiles filesDecoder)
]
div
[]
, div
[ id "file-view" ]
[ text (Debug.toString model) ]
]
[ input
[ type_ "file"
, multiple True
, on "change" (D.map GotFiles filesDecoder)
]
[]
, div
[ id "file-view"
]
[ text (Debug.toString model)
]
]


filesDecoder : D.Decoder (Array File)
filesDecoder =
D.at ["target","files"] (D.array File.decoder)
D.at
[ "target"
, "files"
]
(D.array File.decoder)
Loading