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 Applicative *>, <*, and Alternative <|> operators #66

Merged

Conversation

inamiy
Copy link
Contributor

@inamiy inamiy commented Mar 19, 2016

This pull request adds new Applicative *>, <*, and Alternative <|> operators, which are needed feature in tryswift/TryParsec#11 and all other functional libraries ✨ (See also: #65)

By the way, Argo is currently using the same <|> operator in Alternative.swift#L13 with precedence 140, but this causes continuous monadic operations not working nicely. For example, m1 *> m2 <|> m3 *> m4 doesn't work and must use parenthesis to work around like (m1 *> m2) <|> (m3 *> m4), which is very cumbersome. Since Haskell uses lower precedence for <|>, I have dropped it down to precedence 120 (or, precedence 125 might be a better choice as discussed in tryswift/TryParsec#11).

I also added comments about Haskell's infix operator precedences for comparison with Runes 👀

Expected function type: `f a -> f a -> f a`
Haskell `infixl 3`
*/
infix operator <|> {

Choose a reason for hiding this comment

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

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

@inamiy
Copy link
Contributor Author

inamiy commented Mar 19, 2016

Hmm, it seems below workaround is needed for using Argo when <|> is precedence 120:

return curry(self.init)
  <^> j <| "id"
  <*> (j <| ["userinfo", "name"] <|> j <| "name")  // <- needs parenthesis to work around...
  <*> j <|? "email"

It works better when <|>'s precedence is in range 131...139, but I'm not sure if that value is appropriate since <|> becomes higher priority than <^> and <*>.

@gfontenot
Copy link
Collaborator

@inamiy Is that a problem with the precedence of Argo's <| family of operators? I don't think we've put nearly as much thought into the precedence of those as we have these. I'd be interested to know what the precedence is for .: in Aeson.

It's probably worth noting that we'll probably revisit all of these precedence settings in Swift 3.

@inamiy
Copy link
Contributor Author

inamiy commented Jul 24, 2016

After looking back and simplifying my above old snippet, I found the problem was only in between <*> and <|>.
(So, it's not a problem of Argo's <| operator. <| has 150 and Aeson.(.:) has default infixl 9, which are both high enough)

let d1: Decoded<String> = j <| ["userinfo", "name"]
let d2: Decoded<String> = j <| "name"

return curry(self.init)
  <^> j <| "id"
  <*> (d1 <|> d2)  // <- parenthesis is still required
  <*> j <|? "email"

Because of infixl 4 <*> and infixl 3 <|> in Haskell, adding parenthesis around alternatives is always a requirement to perform applicative style.
That means, ArgoTests/Models/User.swift#L14 should also have parenthesis instead if we adopt this pull request.

This will be a breaking change, so it sounds good to revisit all precedence settings before Swift 3 ;)

@gfontenot
Copy link
Collaborator

Cool, so the gist is that the precedence for <|> is actually currently incorrect in Argo. I think it's reasonable for us to move forward with this since we're going to be bumping major versions across the board with Swift 3 (and the precedence refactoring here) anyway.

@gfontenot
Copy link
Collaborator

@inamiy Do you want to rebase/squash this down on top of current master and then I'll work at merging it?

@inamiy inamiy force-pushed the applicative-alternative-operators branch from 8541c81 to 7a6da19 Compare July 29, 2016 01:43
@inamiy
Copy link
Contributor Author

inamiy commented Jul 29, 2016

@gfontenot Sure! Rebased in 7a6da19. (CI failure seems unrelated)

@gfontenot
Copy link
Collaborator

@inamiy yeah, the CI failure is because of Xcode 8, which doesn't exist on Circle. Totally expected on my part.

@gfontenot gfontenot merged commit 7a6da19 into thoughtbot:master Jul 29, 2016
@gfontenot
Copy link
Collaborator

💥 merged, thanks!

@inamiy inamiy deleted the applicative-alternative-operators branch July 29, 2016 02:01
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.

None yet

3 participants