Skip to content

Comments

Expose a length property for curried functions#477

Merged
dalefrancis88 merged 2 commits intoevilsoft:masterfrom
JamieDixon:curried-expose-length
May 26, 2020
Merged

Expose a length property for curried functions#477
dalefrancis88 merged 2 commits intoevilsoft:masterfrom
JamieDixon:curried-expose-length

Conversation

@JamieDixon
Copy link
Contributor

@JamieDixon JamieDixon commented May 24, 2020

While working through a first-pass on what Async.fromNode might look like with its incoming parameter curried (see #478), I noticed that I was unable to check for a length on incoming curried functions.

With this change in place the code for allowing curried functions with Async.fromNode passes the tests.

What do you think about this change to curry where we expose the length on curried of the original fn that was passed in?

@coveralls
Copy link

coveralls commented May 24, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 0b7c7d7 on JamieDixon:curried-expose-length into ce8f939 on evilsoft:master.

@JamieDixon JamieDixon force-pushed the curried-expose-length branch 2 times, most recently from d2a6d40 to 07d6dda Compare May 24, 2020 16:44
Copy link
Owner

@evilsoft evilsoft left a comment

Choose a reason for hiding this comment

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

Just got one question on the use of configurable, but other than that a welcome addition.

})

Object.defineProperty(curried, 'length', {
configurable: true,
Copy link
Owner

Choose a reason for hiding this comment

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

Why allow this to be configurable?
I would think that this will always be Number and should never be able to be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question and I wish I had a good answer in response.

I literally took inspiration from the following in DevTools:

Object.getOwnPropertyDescriptors(() => {}).length

I'm more than happy to remove this or set it to false.

Copy link
Owner

Choose a reason for hiding this comment

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

I would say, remove it for now. I cannot see a need for it to be configurable.
It defaults to false so no need to explicitly set it (unless you want to communicate that this property is in debate?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent! I've remove the property. Thanks for bringing this one up because I wasn't entirely sure whether it should be included or not 👍

@evilsoft
Copy link
Owner

What do you think about this change to curry where we expose the length on curried of the original fn that was passed in?

This will only be for the "last" fn passed in yeah? For instance this would report as (1), even though it is really (2)

// fn :: String -> Array -> Array
const fn = curry(
  compose(map, objOf)
)

@JamieDixon
Copy link
Contributor Author

@evilsoft Hmmm, that's an interesting one. The length of a function with implicit parameters (arguments or ...args) would be 0 I believe?

If my thinking is on track, the arity of the fn returned by compose is 0 because it's essentially infinite?

@JamieDixon
Copy link
Contributor Author

JamieDixon commented May 25, 2020

There would be a similar discrepancy with functions that employ default values, since the length property only indicates parameters up to the first with a default value.

This number excludes the rest parameter and only includes parameters before the first one with a default value.

Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length

That is to say, I believe this implementation is consistent with expectations as per the spec. What do you think?

@JamieDixon
Copy link
Contributor Author

In addition to this, I wonder whether we could fix the arity of composed functions to the arity of the right-most function in the composition? Just a thought.

@evilsoft
Copy link
Owner

In addition to this, I wonder whether we could fix the arity of composed functions to the arity of the right-most function in the composition? Just a thought.

So this will open up a can of worms, especially if we use this for determining argument application. Because we currently allow an n-ary interface (uncurried) of the right-most function that above function will not be applied properly. When curried, if it reports a length of (2) it will apply it to objOf, passing an Object to map.

We are currently debating limiting arguments to compose/pipe functions to unary for a number of reasons including this behavior. Also it makes functions easier to reason about when composing.

@evilsoft
Copy link
Owner

If my thinking is on track, the arity of the fn returned by compose is 0 because it's essentially infinite?

Yep totes! Good call!

@evilsoft
Copy link
Owner

evilsoft commented May 25, 2020

That is to say, I believe this implementation is consistent with expectations as per the spec. What do you think?

It is 💯, I am just saying that when using this property, we should be aware that this does not pertain to the overall function as a whole, just the current "frame" of the curry stack. So if we are gathering and applying arguments before running a side-effect or something, we cannot use this length as the sole indication that we now have all of our arguments and are ready to apply them.

@JamieDixon JamieDixon force-pushed the curried-expose-length branch from 07d6dda to d29255f Compare May 25, 2020 21:53
@JamieDixon
Copy link
Contributor Author

That is to say, I believe this implementation is consistent with expectations as per the spec. What do you think?

It is 💯, I am just saying that when using this property, we should be aware that this does not pertain to the overall function as a whole, just the current "frame" of the curry stack. So if we are gathering and applying arguments before running a side-effect or something, we cannot use this length as the sole indication that we now have all of our arguments and are ready to apply them.

Oh yes! Absolutely.

Copy link
Owner

@evilsoft evilsoft left a comment

Choose a reason for hiding this comment

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

Unless @dalefrancis88 says 👻 to anything I would say this GTG!

Copy link
Collaborator

@dalefrancis88 dalefrancis88 left a comment

Choose a reason for hiding this comment

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

I think this is fine to add, this only adds value. I know that this has popped up a few times.

I think @evilsoft has covered some really great concerns and they've been addressed, i'm curious to see what will come from this being added.

@dalefrancis88 dalefrancis88 merged commit e5e779a into evilsoft:master May 26, 2020
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.

4 participants