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

ord: add comparison functions #239

Closed
wants to merge 1 commit into from
Closed

ord: add comparison functions #239

wants to merge 1 commit into from

Conversation

davidchambers
Copy link
Member

These functions should be uncontentious.

@kedashoe
Copy link
Member

kedashoe commented Jul 5, 2016

What about ramda/ramda#1497?

@davidchambers
Copy link
Member Author

What about ramda/ramda#1497?

It seems to me there's no perfect solution to this problem. I've wavered, but my current position is that the placeholder is the best of several compromises.

@kedashoe
Copy link
Member

kedashoe commented Jul 5, 2016

Personally I agree with what you wrote at the top of the thread

One can easily write x < y if both values are known, so these functions are primarily useful in conjunction with partial application. Let's optimize for partial application (as we do elsewhere in the library).

I've argued along those same lines when it has come up before on ramda. I honestly never use these functions except when curried.

Would be curious to hear what others think..

@davidchambers
Copy link
Member Author

Pinging @Avaq, @JAForbes, and @scott-christopher for input. I'm in two minds, but not including these functions in the library just because there's no unequivocally correct argument order seems unreasonable. I'm happy to make S.gt(0)(42) equivalent to 42 > 0 if there's sufficient support for switching.

@scott-christopher
Copy link
Member

I think I have always been on the side of wanting these to be thought of as unary functions that return predicates in Ramda, so I would be in favour of S.gt(0)(42) === 42 > 0 here.

@JAForbes
Copy link
Member

JAForbes commented Jul 7, 2016

I'm very much in favour of S.gt(0)(42) being equivalent to 42 > 0 🎆

@CrossEye
Copy link

CrossEye commented Jul 7, 2016

Are these then unary functions? That's always been one possibility for Ramda.

// gt :: Ord a => a -> (a -> Bool)

If they're binary like Ramda's

// gt :: Ord a => a -> a -> Bool

then I think there is equally surprising behavior when these are fully applied. Many users are sure to be confused by gt(0, 42); //=> true.

@davidchambers davidchambers force-pushed the dc-comparison branch 3 times, most recently from 4731e78 to 4e05391 Compare November 6, 2016 19:48
@davidchambers davidchambers force-pushed the dc-comparison branch 2 times, most recently from cfae47a to fce501d Compare November 19, 2016 01:50
@davidchambers davidchambers force-pushed the dc-comparison branch 2 times, most recently from c190574 to 61f23fb Compare December 31, 2016 04:33
@SeanCannon
Copy link

Thanks for even bringing this up - I always felt Ramda's were backwards. I want to be able to do const isGreaterThanThree = R.gt(3); vs R.gt(R.__, 3) which is just ridiculous imo.

@JAForbes
Copy link
Member

JAForbes commented Feb 26, 2017

@CrossEye has made a strong case for ramda's behaviour on gitter.

I still find myself sighing aloud whenever I reach for these functions and need to use the placeholder, or an arrow function But I see value in thinking of these functions as language operators. Recently I've been programming build scripts in livescript with infix syntax, and it makes me thankful for ramda's design in those contexts. I imagine there's plenty of scenarios where retaining an operator inspired api would pay off, and I think I'm just not as good of a programmer as @CrossEye so I've simply not encountered them yet.

If these functions aren't treated as operators, then other functions like concat should reverse the arg order too. And I think a change like that would be frustrating for users. I'd prefer consistency overall, than the minor annoyance of having to use the placeholder in those rare cases that gt etc comes up.

I think I'd be in favour of gt acting like ramda, and bringing in some convenience predicate functions like isGt. I've seen that suggestion come up in the past, and it has grown on me.

Context matters, in the context of a library like sanctuary, I think consistency is pivotal. So I'm at the very least changing my vote to "wavering" like @davidchambers, mostly due to @CrossEye's reasoning.

@gabejohnson
Copy link
Member

@JAForbes It's funny you mention concat, because I have found the arg order there to be inconsistent with the rest of the library. It's even tripped me up a few times.

let append123 = S.concat(S.__, [1,2,3]);
// vs
append123 = S.concat([1,2,3]);

This just may be my bias and not necessarily representative of a community.

But to the point, consistency would require changing sub and div as well I think (along w/ whichever functions Sanctuary has in common w/ the bolded items in ramda/ramda#1497 (comment)). But I question why you would be using binary functions instead of operators in the first place. It makes sense in a transpiled language that allows Haskell-style infix syntax, but no way can we assume that's going to be the common use-case.

I agree w/ @tycho01 in ramda/ramda#1497 (comment)

@davidchambers davidchambers changed the title ord: add S.lt, S.lte, S.gt, and S.gte ord: add comparison functions Mar 2, 2017
@davidchambers
Copy link
Member Author

I've updated this pull request. It now introduces eight functions:

  • lt
  • lte
  • gt
  • gte
  • lt_
  • lte_
  • gt_
  • gte_

Although all eight functions are binary, the unsuffixed ones can be considered unary. lt(10), for example, is the less than 10 function. I chose this naming scheme because it matches the naming of functions such as reduce (which expects a curried binary function) and reduce_ (which expects an uncurried binary function).

If we decide to merge this pull request we should first update existing binary functions accordingly. sub, for example, should be renamed sub_ and a new sub function should be defined such that sub(10) is the subtract 10 function.

What do you think of lt/lt_, sub/sub_, and friends? 👍 / 👎 ?

@KiaraGrouwstra
Copy link

It sorta invokes @TheLudd's remark here, but sounds like the lesser evil.

@davidchambers
Copy link
Member Author

The difference being, @tycho01, that Sanctuary currently does use the _ suffix quite consistently.

@KiaraGrouwstra
Copy link

Fair enough. :)

@kedashoe
Copy link
Member

kedashoe commented Mar 2, 2017

Maybe prefix the underscore (ie _lte) to indicate flipped operators so the current meaning of the underscore suffix can remain distinct? Personally I would rather not include them.. but I guess it could be useful for the type checking?

@davidchambers
Copy link
Member Author

Maybe prefix the underscore (ie _lte) to indicate flipped operators so the current meaning of the underscore suffix can remain distinct?

I see the two uses as related:

  • the _ in lt_ indicates that the function expects all its arguments at once; while
  • the _ in reduce_ indicates that the function argument expects all its arguments at once.

Even if one considers the above a stretch, the _ suffix can at least indicate that the function is looked upon less fondly than its unsuffixed relative. I don't think we should introduce prefixes. We could end up with foo, foo_, _foo, foo$, and $foo if we're not careful. 😜

@kedashoe
Copy link
Member

kedashoe commented Mar 2, 2017

We could end up with foo, foo_, _foo, foo$, and $foo if we're not careful. 😜

ha! Alright, if you want to include them, suffix seems okay to me 👍

@CrossEye
Copy link

CrossEye commented Mar 2, 2017

Hey, why not go for foo_$$___$_$ while we're at it? We could have a whole bitmask of flags for every function. 😄

@gabejohnson
Copy link
Member

What about lte/precedes and gte/succeeds? Not sure which would be unary.

@davidchambers
Copy link
Member Author

I'll close this pull request, as fantasyland/fantasy-land#235 will soon deliver something better. :)

@KiaraGrouwstra
Copy link

KiaraGrouwstra commented May 9, 2017

@CrossEye:

Hey, why not go for foo_$$___$_$ while we're at it? We could have a whole bitmask of flags for every function. 😄

Sanctuary may be safe (pun not intended), but I fear a certain other library may not remain so fortunate!

@gabejohnson
Copy link
Member

I have an updated proposal in #391 (comment) inspired by Haskell's infix operator sections.

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.

8 participants