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

separate R.where into two simpler functions: R.where and R.whereEq #1036

Merged
merged 1 commit into from May 8, 2015

Conversation

davidchambers
Copy link
Member

@davidchambers davidchambers commented Apr 20, 2015

Closes #1032

This is the alternative to #1034. If we decide we want both functions we need to decide what to name them. :)

@@ -50,17 +27,13 @@ describe('where', function() {
});

it('is false if the test object is null-ish', function() {
Copy link
Member Author

@davidchambers davidchambers Apr 20, 2015

Choose a reason for hiding this comment

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

I'd love to remove this special case.

Copy link
Member

@CrossEye CrossEye Apr 20, 2015

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

@davidchambers davidchambers Apr 20, 2015

Choose a reason for hiding this comment

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

@buzzdecafe
Copy link
Member

buzzdecafe commented Apr 20, 2015

IMO where is about specifying an interface that the input object must satisfy, not that the object's individual properties must satisfy in isolation.

@megawac
Copy link
Contributor

megawac commented Apr 21, 2015

I'm fine with this as long as whereEq is included

whereEq = compose(where, mapObject(eq))

@CrossEye
Copy link
Member

CrossEye commented Apr 21, 2015

Damn, it hadn't hit me how straightforward that conversion was:

whereEq = compose(where, mapObject(eq))

Nice.

@svozza svozza mentioned this pull request Apr 21, 2015
@davidchambers
Copy link
Member Author

davidchambers commented Apr 23, 2015

Shall I add R.whereEq?

@CrossEye
Copy link
Member

CrossEye commented Apr 24, 2015

I could be happy with this version of where alongside a whereEq. But I believe @buzzdecafe still might have some reservations about it.

@buzzdecafe
Copy link
Member

buzzdecafe commented Apr 24, 2015

not reservations. just not seeing the point particularly.

@CrossEye
Copy link
Member

CrossEye commented Apr 24, 2015

I say yes, then. Two simple functions beat one complex function hands-down.

@davidchambers davidchambers changed the title simplify R.where separate R.where into two simpler functions: R.where and R.whereEq May 4, 2015
@davidchambers
Copy link
Member Author

davidchambers commented May 4, 2015

I've added R.whereEq.

* spec's own properties, accessing that property of the object gives the same
* value (in `R.eq` terms) as accessing that property of the spec.
*
* `whereEq` is a specialization of [`where`](#where).
Copy link
Member Author

@davidchambers davidchambers May 4, 2015

Choose a reason for hiding this comment

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

Is this valid use of the word specialization?

Copy link
Member

@CrossEye CrossEye May 4, 2015

Choose a reason for hiding this comment

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

I would say so.

@davidchambers
Copy link
Member Author

davidchambers commented May 7, 2015

((🔔)) @buzzdecafe, @CrossEye

@buzzdecafe
Copy link
Member

buzzdecafe commented May 7, 2015

i don't mind the complexity of the original. i don't really object to this, but it doesn't excite me either. i expect this may break a lot of stuff that uses where at present.

this is nice: return where(mapObj(eq, spec), testObj);
but i wonder how it performs? that was the reason for the nastiness of the original.

@davidchambers
Copy link
Member Author

davidchambers commented May 7, 2015

i expect this may break a lot of stuff that uses where at present.

Certainly. In most cases, users will need to replace R.where with R.whereEq:

-R.where({x: 1, y: 2})
+R.whereEq({x: 1, y: 2})

In each of the remaining cases, each non-function value in the spec must be wrapped with R.eq:

-R.where({x: 1, y: 2, z: R.gte(R.__, 0)})
+R.where({x: R.eq(1), y: R.eq(2), z: R.gte(R.__, 0)})

@CrossEye
Copy link
Member

CrossEye commented May 8, 2015

Sorry, I thought I'd made it clear that I was in favor of this.

I do think that sometime soon, we'll have to look at the basic implementation. If I'm not mistaken, in the Great Curry Debacle of 2014, we reset this one in such a way that it might have lost some of the benefits that had been gained by the initial manual currying we had done. This one was the function that showed us the need for changes to our old currying, and it had some interesting behavior...

buzzdecafe added a commit that referenced this pull request May 8, 2015
separate R.where into two simpler functions: R.where and R.whereEq
@buzzdecafe buzzdecafe merged commit 75cc61b into ramda:master May 8, 2015
@megawac
Copy link
Contributor

megawac commented May 8, 2015

Let's just make sure this is bold, highlighted, underlined in change log as
it will break a good amount of code
On May 8, 2015 10:46 AM, "Michael Hurley" notifications@github.com wrote:

Merged #1036 #1036.


Reply to this email directly or view it on GitHub
#1036 (comment).

@buzzdecafe
Copy link
Member

buzzdecafe commented May 8, 2015

yep, totally agree

@davidchambers davidchambers deleted the where-pred branch May 8, 2015
@buzzdecafe
Copy link
Member

buzzdecafe commented May 8, 2015

not sure what to make of this: http://jsperf.com/mapobj-test

@davidchambers
Copy link
Member Author

davidchambers commented May 8, 2015

I could certainly get behind a more efficient implementation. It was API complexity this pull request sought to reduce.


return _satisfiesSpec(spec, parsedSpec, testObj);
for (var prop in spec) {
if (_has(prop, spec) && !spec[prop](testObj[prop])) {
Copy link
Contributor

@megawac megawac May 9, 2015

Choose a reason for hiding this comment

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

Any reason you're not using keys here (it'd be faster)?

Copy link
Member Author

@davidchambers davidchambers May 9, 2015

Choose a reason for hiding this comment

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

Sounds like a good idea to me!

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.

simplify R.where
4 participants