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

R.propIs() and R.propEq() have inconsistent argument order #2428

Open
helios1138 opened this issue Jan 5, 2018 · 10 comments
Open

R.propIs() and R.propEq() have inconsistent argument order #2428

helios1138 opened this issue Jan 5, 2018 · 10 comments

Comments

@helios1138
Copy link

helios1138 commented Jan 5, 2018

  • R.propIs() and R.propEq() are named similarly enough that it's expected that they would place the prop name argument in the same place
  • but R.propIs() takes prop name as the last argument while R.propEq() takes it as the last one
  • ideally, R.propIs() should reverse it's argument order or reverse its name to something like R.isProp() or vice versa for the other one
  • I think these functions are not very frequently used with partial application, they usually make sense as part of some conditional statements where readability is important
    • e.g. if you're reading the code and R.propEq() takes two strings you can't quickly understand which of them is the prop name and which is the value unless the argument order is intuitively consistent with function names across the library
  • same problem with R.propOr() I guess
@CrossEye
Copy link
Member

CrossEye commented Jan 5, 2018

It's a good question. I believe propOr, which doesn't in general return a boolean, is a different case. But propSatisfies should also be discussed.

I remember waffling on the argument order for propEq. For most functions, it's simply obvious what order is best for currying. That one wasn't. When we later added propIs and propSatisfies, the order for those seemed obvious again:

propIs(Number) ~~> name => obj => is(Number)(prop(name, obj))
propSatisfies(isOdd) ~~> name => obj => idOdd(prop(name, obj))

These seem so obviously right that I'm loathe to change them. propEq doesn't have as clear an answer to me. But if we were to switch it to

propEq(42) ~~> name => obj => equals(42)(prop(name, obj))

we would certainly add some consistency. I think it would be worth hearing from others whether they think this consistency is worth breaking backward compatibility. What do you say @ramda/core?

@TheLudd
Copy link
Contributor

TheLudd commented Jan 5, 2018

propEq is one of out most used functions from ramda and its argument order is just the way it needs to be.

const hasEmail = propEq('email')
const isMe = hasEmail('ludwig@mediatool.com')
const me = find(isMe, userArray)

The order of propEq was actually changed in one release and then changed back because of many complaints.

The prop you are checking is the most static argument, the value to compare against is the second most static. Often the second value will be based on user input or database values but sometimes they are constants. And the object you compare against is almost always user input.

For some function, argument order can be tricky. I really do not think this is in the case for propEq but in any case, to me the argument for a specific order should never be "there is a similarly named function, lets copy the order from that one".

@CrossEye
Copy link
Member

CrossEye commented Jan 5, 2018

@TheLudd:

The reverse seems equally likely in my experience:

const uninitialized = flip(propEq)(null)
const noName = uninitialized('name')

noName({age: 10, name: null}) //=> true

The order of propEq was actually changed in one release and then changed back because of many complaints.

That doesn't surprise me, although I don't recall it.

the argument for a specific order should never be "there is a similarly named function, lets copy the order from that one".

I think it's a worthy consideration if the order is not already dictated by other factors, which, of course, it usually will be. Consistency is quite useful.

@TheLudd
Copy link
Contributor

TheLudd commented Jan 5, 2018

The reverse seems equally likely in my experience:

Really? I mean sure it can happen but is it as common as the other case? Do you have several props that you all want to check for a specific value?

If we look at projects that use Ramda, do we find that propEq is flipped more times than it is not?
I would be very surprised if the answer was yes. If it was however then it should be changed.

@TheLudd
Copy link
Contributor

TheLudd commented Jan 5, 2018

Consistency is quite useful.

For any function I would say that if it is flipped more often than it is not, that would be the only reason to change order. I mean, consider that you would need to do flip(fnX) in 90% of the cases that you used fnX would you really buy in to the consistency argument?

Consistency in the form of xBy is a predicate taking one argument and xWith takes two arguments and returns a boolean, that is where consistency makes sense to me.

@CrossEye
Copy link
Member

CrossEye commented Jan 5, 2018

Really? I mean sure it can happen but is it as common as the other case? Do you have several props that you all want to check for a specific value?

Often enough that I have many times had concerns about the ordering. I'm guessing that it's probably 3 : 2 in favor of the existing order in my own uses. (I guess I was exaggerating about "Equally likely".)
That's not enough to make it crystal clear to me. Do you agree that the ordering for propIs / propSatisfies is already correct?


For any function I would say that if it is flipped more often than it is not, that would be the only reason to change order. I mean, consider that you would need to do flip(fnX) in 90% of the cases that you used fnX would you really buy in to the consistency argument?

Absolutely. The only way this would come to be a major factor is if it was hovering somewhere close to 50%.

@TheLudd
Copy link
Contributor

TheLudd commented Jan 5, 2018

I am surprised at your 3:2 ratio. I did a search in my company's code and found 242 cases of propEq, non of them flipped.

I have never used propIs or propSatisfies but they look right.
If I wanted to check types of different props I do beleive that I would have several different props and check that they are of type Number lets say. I do not expect to have one single prop and check that against different types.

// Makes sense
const propIsNumber = propIs(Number)
const ageIsNumber = propIsNumber('age')
const scoreIsNumber = propIsNumber('score')

// Does not make sense
const ageIs = propIs('age')
const ageIsNumber = ageIs(Number)
const ageIsString = ageIs(String)
const ageIsArray = ageIs(Array)

@helios1138
Copy link
Author

helios1138 commented Jan 5, 2018

I think the bigger point here is that there should be a consistency between naming and argument order (propIs(prop, is) and isProp(is, prop) but can be reversed and still consistent) and between different functions with similar-looking names (propIs, propEq).

What that order exactly is might be less important (since you can always flip it). Simply choosing the order based on the popularity of use cases per function might not be a good idea and detrimental to the overall consistency which is really important for a library with ~200 functions. Having the ability to flip or having flipped functions prepackaged (like having both isProp and propIs) should help with that instead.

@Bradcomp
Copy link
Member

Bradcomp commented Jan 5, 2018

I think that getting the order right in terms of ergonomics is much more important than basing the order on the function name. isProp reads to me as asking a question - is this a property of that? - whereas propIs and propSatisfies are clearly asking if the prop is something or satisfies something. I think those two functions, given their functionality, have the correct parameter order within the overall philosophy of Ramda. I've never had to look up the parameter order for those two.

propEq is trickier, however I do buy the arguments made by @TheLudd. For my part, I don't see a clear preference there, and often have to look up the type signature for that one. I agree that functionally the order makes sense, but the name does maybe imply that the first 2 arguments might be flipped...

For my part though I wouldn't change it, as I can't think of a time that I have flipped it, even though I use it fairly often.

@CrossEye
Copy link
Member

CrossEye commented Jan 5, 2018

I think part of the reason I don't get frustrated with this one is that I most often partially apply the first two parameters. I don't do a lot with either foo = propEq(name) or bar = flip(propEq)(val), but usually with baz = propEq(name, val). And then I don't need to flip, only remember the parameter order.

@helios1138: While I'm sympathetic to your suggestion, I would never let the inter-function consistency overrule the ergonomics of individual functions. And while we strive for some consistency in the words used in function names (such as *With vs *By), this has never been a dominant theme.

That one could just flip a function is not a major help, to my mind. We want these to be as intuitive as possible (even if Underscore users groan! 😄) Excessive flipping is an API failure to my mind.

The only reason I wobble here a bit is that I'm not particularly convinced that propEq has a clear-cut choice. I respect what @TheLudd is saying about it. My "less often" is his "never", but we're still on the same side of this. But there is a naming problem, which we can't simply reverse, since we already use eqProps for something entirely different.

So if we were to start from scratch, I might like a different name for propEq, but I can't see changing it now.

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

No branches or pull requests

4 participants