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

Add the "andThenN" family of functions #50

Merged
merged 7 commits into from
Mar 8, 2020
Merged

Add the "andThenN" family of functions #50

merged 7 commits into from
Mar 8, 2020

Conversation

ursi
Copy link
Contributor

@ursi ursi commented Feb 13, 2020

No description provided.

@skyqrose
Copy link
Collaborator

Sorry it took me so long to get around to reviewing this.

There's some discussion on slack, but since that's not permanent, I'll recap it here:

brianhicks: I think the recommendation there would be to use andMap.

Just (+) |> andMap (Just 1) |> andMap (Just 2) == Just 3
Just (+) |> andMap Nothing |> andMap (Just 2) == Nothing
Just (+) |> andMap (Just 1) |> andMap Nothing == Nothing
but the andThenN names would be easier to discover, certainly!

@ursi: An example of using andThen2: https://ellie-app.com/82ds5fd624Sa1
@pzp1997: Could be done with map2 instead like this: https://ellie-app.com/82yr5MZmt5Da1
@ursi: Another example: https://ellie-app.com/82BZcXjXfp3a1


I see in the second example you can't use map2 because you don't know if the parsing will fail until you have both values.

But even in that case, you can use andMap/mapN and join as a workaround

{-| same as in your Ellie -}
f : String -> String -> Maybe Int
f value1 value2 =
    map2
        (+)
        (String.toInt (value1 ++ value2))
        (String.toInt (value2 ++ value1))

combineToInt : Int -> Array String -> Maybe Int
combineToInt index array =
    Maybe.map2
        f
        (Array.get index array)
        (Array.get (index + 1) array)
        |> Maybe.Extra.join

(I haven't compiled or run that, so it's possible that I'm missing something.)

Generally, I'd like to be pretty conservative about what goes into this package, especially if it's a big family of many functions. But I could be convinced that the readability/discoverability/usability of andThenN rather than join is worth it.

In your real world use case, can you check the values before hand like the first example, or do you need both to know if it will fail like the 2nd?

Do you know of any other use cases?

Are there examples of similar andThenN functions in other packages or languages?

@pzp1997
Copy link

pzp1997 commented Feb 23, 2020

@skyqrose I agree with you that it would be helpful to see more use cases or precedent for andThenN in other languages. With regards to your comment about being able to use mapN and join as a workaround, @ursi's proposed implementation of andThenN is essentially mapN >> join (since join is equivalent to andThen identity). Personally, I don't think andThenN is clearer than mapN >> join but perhaps I am biased as I am very used to things as they are.

@ursi
Copy link
Contributor Author

ursi commented Feb 23, 2020

Generally, I'd like to be pretty conservative about what goes into this package, especially if it's a big family of many functions.

I totally understand this mentality, and if this isn't deemed essential enough to be included then so be it. Personally I find using them more readable. I can always just put them in their own package. Perhaps I could even make a general andThenN package that had andThenNs for more than just Maybe.

Do you know of any other use cases?

One solid use case for these is nesting them. Using andThen identity/join once isn't the worst thing in the world (although personally I don't want to if I don't have to) but when nesting, you get to a point where you have to call it 2 or 3 times in a row

Are there examples of similar andThenN functions in other packages or languages?

Elm is my first functional language so I cannot comment on this. I just saw myself commonly using mapN with andThen idenity and decided to make a helper function for it, and when I made the type signature I realized it was just a generalized andThen.

@ursi
Copy link
Contributor Author

ursi commented Feb 23, 2020

Here's a more involved example https://ellie-app.com/87g4M6W9Fb7a1

pointColor : Float -> Int -> Array Point -> Color
pointColor time index array =
    Maybe.withDefault black <|
        andThen2
            (\p1 p2 ->
                andThen2
                    (\v1 v2 ->
                        if v1 <= time && time < v2 then
                            Just p1.color

                        else
                            Nothing
                    )
                    p1.value
                    p2.value
            )
            (Array.get index array)
            (Array.get (index + 1) array)

I have a time and an array of points with values. I want to use the point's color if the time is after the point, but before the next point, and I want to use black otherwise.

@prikhi
Copy link
Member

prikhi commented Feb 23, 2020

Are there examples of similar andThenN functions in other packages or
languages?

Elm is my first functional language so I cannot comment on this. I just saw
myself commonly using mapN with andThen idenity and decided to make a helper
function for it, and when I made the type signature I realized it was just a
generalized andThen.

It's not really necessary in Haskell because of do notation or the >>=
operator:

-- With `<$>` and `<*>` we don't even need the `mapN` functions
f :: String -> String -> Maybe Int
f v1 v2 =
    (+) 
        <$> (readMaybe (v1 ++ v2))
        <*> (readMaybe (v2 ++ v1))

-- `do` notation for the Maybe type pulls out the Just value or returns Nothing
-- for the whole block if the value on the right side of the `<-` is a Nothing.
combineToInt index array = do 
    v1 <- get index array
    v2 <- get (index + 1) array
    f v1 v2

Or the more complex example:

getPointColor time index array = fromMaybe black $ do 
    p1 <- get index array
    p1Value <- pointValue p1
    p2Value <- get (index + 1) array >>= pointValue
    if p1Value <= time && time <= p2Value then 
        Just $ pointColor p1
    else 
        Nothing

Seems like the only other andThenN functions live in random-extra:
https://klaftertief.github.io/elm-search/?q=andThen2

@ursi
Copy link
Contributor Author

ursi commented Mar 7, 2020

What exactly are we waiting on for this to be closed/merged?

@skyqrose
Copy link
Collaborator

skyqrose commented Mar 8, 2020

Ah, sorry for the delay, and thanks for the reminder.

I've been searching around (example searches: 1 2 3 ) and I'm seeing enough places where people reimplement these (for Maybe and for other data types) that I think it's worth including these.

This package I saw seems particularly relevant: ccapndave/elm-flat-map. It does what you were thinking of in having one package with andThenN functions for a bunch of different data types.

I'll do some organization and cleanup. (I think it should go before the Applicative Functions section, and it needs tests) and then I'll merge it.

In all my searching I've only seen one place that used a andThen4, and never seen a andThen5 or more, so I think we should stop at 4. I'll take care of that, too.

@skyqrose
Copy link
Collaborator

skyqrose commented Mar 8, 2020

@ursi I pushed some changes. If you're happy with them I'll merge it.

There is no need for the functions to be implemented as a combination of
'Maybe.map' and 'Maybe.andThen identity'
@ursi
Copy link
Contributor Author

ursi commented Mar 8, 2020

@skyqrose I just went up to five because Maybe.map went up to five. And speaking of that, I realized when looking through implementations that using Maybe.map was not necessary and was actually just introducing and extra unneeded step, so I re-implemented them just using case ... of. If you're fine with that then I'm fine with merging 👍

@skyqrose skyqrose merged commit 882815e into elm-community:master Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants