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.

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

Is handling of function application followed by infix operators okay? #33

Open
avh4 opened this issue Oct 21, 2015 · 13 comments
Open

Is handling of function application followed by infix operators okay? #33

avh4 opened this issue Oct 21, 2015 · 13 comments

Comments

@avh4
Copy link
Owner

avh4 commented Oct 21, 2015

                    vboxlist
                        (text "| ") ", "
                        empty
                        pair pairs
                        |> indent 4

the function arguments and the infix operators get indented to the same level, even though they are at different levels in the AST.

@Apanatshka
Copy link

I prefer the infix operator to not be indented:

f1
    arg11
    arg12
    arg13
|> f2 arg21

alpha
+ beta
  * gamma

@avh4 avh4 changed the title Is handling of function application + infix operators okay? Is handling of function application followed by infix operators okay? Oct 23, 2015
@rtfeldman
Copy link

This is pretty interesting. @Apanatshka's version implies not indenting for infix operators in general.

Here's something we have in our code base:

model.topicsById
    |> Dict.get topicId
    |> Maybe.map (not << .premium)
    |> Maybe.withDefault False

With your suggestion, it would be:

model.topicsById
|> Dict.get topicId
|> Maybe.map (not << .premium)
|> Maybe.withDefault False

Those both look very readable to me, although the latter saves space and arguably has some consistency benefits.

What do you think @avh4 @evancz?

@rtfeldman
Copy link

On second thought, that looks weird with non-pipes...

someNumber
+ anotherNumber
+ aThirdNumber

I vote leaving it the way it currently works, including the OP question about indentation levels - it's not my favorite thing, but seems better than the alternatives.

@Apanatshka
Copy link

It looks a little odd, but I still like my way better because it solves the problem in this issue and it's how I style my pipeline code.

@evancz
Copy link
Collaborator

evancz commented Nov 13, 2015

F# people often do their pipes that way. I never do that though. I don't have a strong opinion here though.

@rtfeldman
Copy link

Do we want to style pipelines different from other infix operators? What would be the criteria for that - special case |> or just do it for any infix operator that starts with a |? In the latter case, does that mean || works differently?

Seems like high potential for added complexity...

@deadfoxygrandpa
Copy link
Contributor

deadfoxygrandpa commented Dec 25, 2015

edit: Comments relate to #187 Not sure if this belongs here, but I had a block of code like this:
person : Random.Generator Person
person =
    sex `andThen` \s ->
    weight `andThen` \w ->
    height `andThen` \h ->
    Random.Extra.constant (Person s w h)

and elm-format turned it into this:

person : Random.Generator Person
person =
    sex
        `andThen` \s ->
                    weight
                        `andThen` \w ->
                                    height
                                        `andThen` \h ->
                                                    Random.Extra.constant (Person s w h)

Except it was even longer. I don't feel like this is a good formatting style for the andThen patterns that we have in some modules.

@cpdean
Copy link

cpdean commented Jan 4, 2016

edit: Comments relate to #187 I have a similar gripe with how `andThen` is handled. Maybe the code i'm reading in elm-oracle isn't idiomatic?
    Search sourceFile query ->
      tryCatch emitError <|
        loadSource sourceFile
          `andThen` \source -> loadDeps
          `andThen` \deps -> Task.fromResult (parseDeps deps)
          `andThen` \docPaths -> downloadDocs docPaths
          `andThen` \_ -> loadDocs docPaths
          `andThen` \depsDocs -> getProjectDocs
          `andThen` \projectDocs -> Task.succeed (projectDocs ++ depsDocs)
          `andThen` \docs -> Console.log (Oracle.search query source docs)

turns into

        Search sourceFile query ->
            tryCatch emitError
                <| loadSource sourceFile
                `andThen` \source ->
                            loadDeps
                                `andThen` \deps ->
                                            Task.fromResult (parseDeps deps)
                                                `andThen` \docPaths ->
                                                            downloadDocs docPaths
                                                                `andThen` \_ ->
                                                                            loadDocs docPaths
                                                                                `andThen` \depsDocs ->
                                                                                            getProjectDocs
                                                                                                `andThen` \projectDocs ->
                                                                                                            Task.succeed (projectDocs ++ depsDocs)
                                                                                                                `andThen` \docs -> Console.log (Oracle.search query source docs)

@rtfeldman
Copy link

rtfeldman commented Jan 4, 2016

edit: Comments relate to #187 I recommend using `andThen` with `|>` instead of backticks, like so:
    andThen = flip Task.andThen

    Search sourceFile query ->
      tryCatch emitError
         <| loadSource sourceFile -- I'd refactor this out; 99% of the time, when I use <| there's a better way
          |> andThen (\source -> loadDeps)
          |> andThen (\deps -> Task.fromResult (parseDeps deps))
          |> andThen (\docPaths -> downloadDocs docPaths)
          |> andThen (\_ -> loadDocs docPaths)
          |> andThen (\depsDocs -> getProjectDocs)
          |> andThen (\projectDocs -> Task.succeed (projectDocs ++ depsDocs))
          |> andThen (\docs -> Console.log (Oracle.search query source docs))

@avh4
Copy link
Owner Author

avh4 commented Jan 4, 2016

edit: Comments relate to #187 @rtfeldman I don't think that works in this case because some of the lambdas refer to parameters of previous lambdas. Can the pipeline work for cases like that?

@deadfoxygrandpa
Copy link
Contributor

deadfoxygrandpa commented Jan 4, 2016

edit: Comments relate to #187 @rtfeldman I just tested this and it turned my example into
person : Random.Generator Person
person =
    let
        andThen = flip Random.andThen
    in
        sex
            |> andThen (\s -> weight)
            |> andThen (\w -> height)
            |> andThen (\h -> Random.Extra.constant (Person s w h))

Which doesn't compile, because s w and h are not found in the final function. I can change it to:

person : Random.Generator Person
person =
    let
        andThen = flip Random.andThen
    in
        sex
            |> andThen (\s -> weight
            |> andThen (\w -> height
            |> andThen (\h -> Random.Extra.constant (Person s w h))))

Which compiles, but elm-format changes it to

person : Random.Generator Person
person =
    let
        andThen = flip Random.andThen
    in
        sex
            |> andThen
                (\s ->
                    weight
                        |> andThen
                            (\w ->
                                height
                                    |> andThen (\h -> Random.Extra.constant (Person s w h))
                            )
                )

Which isn't really much better.

@rtfeldman
Copy link

rtfeldman commented Jan 8, 2016

edit: Comments relate to #187 Ah, good point - I totally missed that!

@avh4 avh4 added this to the 0.4.0 public beta milestone May 25, 2016
@avh4
Copy link
Owner Author

avh4 commented May 25, 2016

Discussion of the andThen + lambdas problem should move to #187.

@avh4 avh4 modified the milestones: 1.0.0 public release, 0.6.0 public beta Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants