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

identify R.__ by special property rather than by identity #1156

Merged
merged 1 commit into from
Jun 9, 2015

Conversation

davidchambers
Copy link
Member

Closes #1052

The '@@functional/placeholder' property name was proposed by @paldepind in #1052 (comment).


If we decide to merge this pull request, I wonder how likely it is that the Underscore contributors would agree to add this property to the _ object.

@buzzdecafe
Copy link
Member

🐄
let's do it, and adjust to reactions of others later

@megawac
Copy link
Contributor

megawac commented Jun 5, 2015

Heads up, there used to be a deopt associated with property names that cannot be accessed by dot notation (v8)

@CrossEye
Copy link
Member

CrossEye commented Jun 6, 2015

I think this is a good pass at this. Later, we may want to consider a Symbol instead, for those environments supporting them, but that's not urgent.

@megawac: although I doubt that would stop us (as this style of property name for such interfaces if getting to be a defacto standard, ISTM), I am curious to hear more about that de-opt. Do you have any references for it?

assert.deepEqual(g(_, _, _)(_, _)(_)(1, 2, 3), [1, 2, 3]);
assert.deepEqual(g(_, _, _)(1, _, _)(_, _)(2, _)(_)(3), [1, 2, 3]);
});

Copy link
Member

Choose a reason for hiding this comment

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

All this testing looks superfluous to me. curry is based on curryN and curryN is tested for the exact same behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow. We need these tests to ensure we're not comparing to R.__ by identity, don't we?

Copy link
Member

Choose a reason for hiding this comment

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

My point is that these tests should be for curryN. You've made no change to curry – yet you've changed the tests for curry but not for curryN.

Copy link
Member

Choose a reason for hiding this comment

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

Well, since curry is public and its behavior has changed, its tests should probably reflect that. The fact that it is implemented in terms of curryN is not a public guarantee, so our tests shouldn't assume this.

OTOH, we probably also need similar tests for curryN, as that one is also public.

Copy link
Member

Choose a reason for hiding this comment

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

That is not an unreasonable way to see it. I agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added tests for R.curryN. Thanks for pointing out this omission, @paldepind.

Copy link
Member

Choose a reason for hiding this comment

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

Hehe. It was not really my initial intent. But I'm glad it lead to something useful.

@CrossEye
Copy link
Member

CrossEye commented Jun 9, 2015

🌿

buzzdecafe added a commit that referenced this pull request Jun 9, 2015
identify R.__ by special property rather than by identity
@buzzdecafe buzzdecafe merged commit 3b3e9e8 into ramda:master Jun 9, 2015
@davidchambers davidchambers deleted the placeholder branch June 9, 2015 21:04
@jdalton
Copy link

jdalton commented Jun 10, 2015

If we decide to merge this pull request, I wonder how likely it is that the Underscore contributors would agree to add this property to the _ object.

Not likely from my end. Lodash/Underscore support customizable placholders in a different way, based on the .placeholder property on methods with placeholder support, e.g. _.partial.placeholder.

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

6 participants