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

Validate array argument for promise-array related functions #35

Closed
jsor opened this issue Jul 6, 2015 · 6 comments
Closed

Validate array argument for promise-array related functions #35

jsor opened this issue Jul 6, 2015 · 6 comments

Comments

@jsor
Copy link
Member

jsor commented Jul 6, 2015

Reject all() / race() / any() / some() / map() / reduce() when called with a non-array.

all(null)
    ->otherwise(function(\InvalidArgumentException $e) {
        assertEquals('Expecting an array or a promise which resolves to an array, got NULL instead.' , $exception->getMessage());
    });
@jsor jsor added this to the v3.0 milestone Jul 6, 2015
@clue
Copy link
Member

clue commented Sep 7, 2015

👍 for rejecting invalid input

But then again what is valid input here? :-)

@jsor
Copy link
Member Author

jsor commented Sep 8, 2015

At the moment, it is an array or a promise which resolves to an array. Passing anything other produces

all(null)
    ->then(function($value) {
        assertEquals([], $value);
    });

which i think is not an expected behaviour.

@clue
Copy link
Member

clue commented Sep 8, 2015

which i think is not an expected behaviour

Yeah, I agree that we should handle this is in a more sane way 👍

However, now that we're already considering what is valid input, I would suggest using a stricter definition. For example, ES6-style promises limit the parameter to be an array of promises (or values which are considered like resolved promises): https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Promise/all (which IMHO makes sense to me).

As such, does it make sense to pass a single Promise which resolves to an array of values? Do we have a valid use case for this?

@jsor
Copy link
Member Author

jsor commented Sep 8, 2015

As such, does it make sense to pass a single Promise which resolves to an array of values? Do we have a valid use case for this?

I've never used a single input promise.

Options:

  • No change, allow input promise which resolves to an array, requires rejection with InvalidArgumentException for non-array input
  • Typehint against array, does not require rejection for non-array input
  • Allow array and \Traversable input, no typehint possible, requires rejection with InvalidArgumentException for non-array input

I'm unsure on this atm.

@clue
Copy link
Member

clue commented Sep 8, 2015

No change, allow input promise which resolves to an array, requires rejection with InvalidArgumentException for non-array input

👎 on this unless we find a valid use case

  • Typehint against array, does not require rejection for non-array input
  • Allow array and \Traversable input, no typehint possible, requires rejection with InvalidArgumentException for non-array input

IMO both sound sane…

Some (random) thoughts:

  • On success, the all() method resolves with an array, so for consistency it may make sense to require an input array
  • Traversable and Iterator have valid use cases, though I'm failing to see how these would be beneficial here. Also, converting via iterator_to_array() is trivial.
  • Permitting several input types means we have to use additional runtime checks which add to complexity. Also, this makes it harder to use type guessing (IDEs).
  • Typehinting against array is trivial and requires no boilerplate. It probably fulfills 80%+ of the use cases anyway and converting to an array is trivial.

As such, I'm leaning towards using an array typehint 👍

@jsor
Copy link
Member Author

jsor commented Sep 8, 2015

As such, I'm leaning towards using an array typehint

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants