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

Implement applySpec #1592

Merged
merged 1 commit into from Feb 15, 2016
Merged

Implement applySpec #1592

merged 1 commit into from Feb 15, 2016

Conversation

asaf-romano
Copy link
Member

Closes #1591

return curryN(reduce(max, 0, pluck('length', values(spec))),
function() {
var args = arguments;
return map(function(f) { return f.apply(null, args); }, spec);
Copy link
Member

Choose a reason for hiding this comment

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

Could we use R.apply here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do in the round.

By the way, I must say that this line is confusing:
https://github.com/ramda/ramda/blob/master/src/apply.js#L24

If it's intentionally trying to support mapping this to the global object, I guess that's fine*, but it better be documented (in the code), because it's easy to overlook that.


  • Note that this makes the caller environment significant:
(function f() { return require("ramda").apply(function() { return this; }, []);  })(); // => Node's global object;
(function f() { "use strict"; return require("ramda").apply(function() { return this; }, []);  })() // => undefined

@CrossEye
Copy link
Member

By the way, I must say that this line is confusing:
https://github.com/ramda/ramda/blob/master/src/apply.js#L24

If it's intentionally trying to support mapping this to the global object, I guess that's fine*, but it better be documented (in the code), because it's easy to overlook that.

That's not it. Most of our functions that manipulate functions use a similar technique to make it easier to use the generated functions in an OO context. It's pretty difficult right now to come up with a good example of why someone would want to do that with apply, but the point is that if the function supplied uses 'this' and the function generated ends up being used as an Object method, then because of this technique, we properly keep track of the this being used when calling the function.

This has always been more of a nice-to-have feature than something driving our API. But if it doesn't get in our way, it helps smooth the way for this library to interact with other JS code the user may have to work with.

@asaf-romano
Copy link
Member Author

@CrossEye I see, I think it's broken though: http://bit.ly/1ZpEC37

@CrossEye
Copy link
Member

Sure enough. I imagine some change to curry2 has caused this.

When I have some time, I'll try to dig into it and see if I can see why.

@asaf-romano
Copy link
Member Author

@asaf-romano
Copy link
Member Author

The same issue applies to _curry3.

@asaf-romano
Copy link
Member Author

Feedback would be nice ;)

@CrossEye
Copy link
Member

Feedback would be nice ;)

Thanks for the poke. Behind, as ususal.

🌿 I really like this. I see it as quite useful, and not easily achievable with anything else we have.

I'm not sure I like the implicit recursion. It might be better to make it explicit:

R.applySpec({ unnested: R.always(0), nested: R.applySpec({ sum: R.add }) })(1, 2);
//=> {unnested: 0, nested: {sum: 3}}

But I'm not certain about this, mostly thinking aloud.

@asaf-romano
Copy link
Member Author

The library is not consistent elsewhere:

R.evolve({ a: { b: R.inc } }, { a: { b: 1 } }) // => {"a": {"b": 2}}
R.where({ a: { b: R.gt(R.__, 0 ) } }, { a: { b: 0 } }) // throws "spec[prop] is not a function"

Practically speaking, it's been quite a few times where({ currentBehavior: { where: disappointedMe } }) (and somewhat surprised me, given how evolve works); On the other hand, I do find the recursive behavior of evolve very powerful.

As for our own implementation of applySpec: initially it wasn't recursive, but it quickly became clear that the recursive use case is very sound. Not exactly our use case, but consider a function that constructs a nicely nested object out of a flat DB-row result, something like this:

  const getUserInfo = pipeP(
    ...,
    applySpec({
      credentials: {
        userName: prop("user_name"),
        passowrd: pipe(prop("passowrd_enc"), ...)
      },
      lastActive: pipe(prop("last_active"), toDate),
      stats: {
         accessCount: pipe(...),
         ....
      },
      ...
    })
  );

If the "sub-specs" for credentials, stats and other properties aren't reusable (i.e. those objects don't stand by themselves), a non-recursive implementation would just add some unnecessary noise, somewhat hiding the author's intentions.

@CrossEye
Copy link
Member

It was that inconsistency I knew we already had that was bothering me, although I hadn't yet tried to figure out exactly where each style was used.

I believe you're right. The recursive one is much more powerful for a spec, so long as we're only passing functions. where evolved from a different direction, and we couldn't have done this before #1036. But I would be in favor of making where recursive as well.

🌿

Any objections, anyone?

@CrossEye CrossEye mentioned this pull request Jan 30, 2016
@CrossEye CrossEye mentioned this pull request Jan 31, 2016
CrossEye added a commit that referenced this pull request Feb 15, 2016
@CrossEye CrossEye merged commit 28c176f into ramda:master Feb 15, 2016
@buzzdecafe buzzdecafe mentioned this pull request Mar 26, 2016
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