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

Reject some() (and any()) if the input array contains not enough items #34

Merged
merged 8 commits into from
Mar 30, 2016

Conversation

jsor
Copy link
Member

@jsor jsor commented Jul 6, 2015

This fixes a bug where some() never resolves if the input array contains not enough items.

some([1, 2, 3], 4)->then(function() {
    // Never called
});

@jsor
Copy link
Member Author

jsor commented Sep 7, 2015

ping @cboden @clue @WyriHaximus

->with(
$this->callback(function($exception){
return $exception instanceof RangeException &&
'Input array must contain at least 1 item but contains only 0 items.' === $exception->getMessage();
Copy link
Member

Choose a reason for hiding this comment

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

The documentation seems to suggest otherwise. IMO it makes sense to reject this with a RangeException, but we should probably also add this special case to the README.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a note the the README (e29472f).

@clue
Copy link
Member

clue commented Sep 7, 2015

See remark about documentation for any(), otherwise LGTM 👍

@clue
Copy link
Member

clue commented Sep 8, 2015

README now LGTM :)

However, after further consideration, I think this should probably throw a subclass of LogicException rather than RuntimeException. After all this is in fact a logic error which could be detected at "compile time" (i.e. it does not really depend on a runtime decision). What's your view on this?

@jsor
Copy link
Member Author

jsor commented Sep 8, 2015

Good point. I've used RangeException after discovering this bug while looking through the bluebird code and extended the SPL RangeException (which in turn extends RuntimeException).

I think it would be strange to use the name RangeException and extend LogicException because this would differ from the SPL.

Maybe we should use another name...any suggestions?

@@ -515,6 +515,9 @@ will be the resolution value of the triggering item.
The returned promise will only reject if *all* items in `$promisesOrValues` are
rejected. The rejection value will be an array of all rejection reasons.

The returned promise will also reject with a `React\Promise\Exception\RangeException`
if `$promisesOrValues` contains 0 items.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would UnderflowException or LengthException, both of which extend LogicException, make sense here?

Edit: just realised that UnderflowException extends RuntimeException.

Copy link
Member Author

Choose a reason for hiding this comment

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

LengthException might be an option...

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to LengthException in cd6bb56.

@jsor jsor added this to the v3.0 milestone Jan 21, 2016
@jsor
Copy link
Member Author

jsor commented Jan 21, 2016

I'm planning to add this to 3.0.

@jsor jsor removed this from the v3.0 milestone Mar 25, 2016
@jsor
Copy link
Member Author

jsor commented Mar 25, 2016

Any objections against including this in a 2.4.0 release?

@jsor jsor merged commit 137f007 into master Mar 30, 2016
@jsor jsor deleted the some-underflow branch March 30, 2016 07:22
@cboden
Copy link
Member

cboden commented Mar 30, 2016

Technically isn't this an API break?

@jsor
Copy link
Member Author

jsor commented Mar 30, 2016

I think (and iirc we agreed on IRC) that this is a bug fix, because without this fix

some([1, 2, 3], 4)
    ->then(function() {
        // Never called
    })
    ->otherwise(function() {
        // Never called
    });

simply does nothing. It neither calls the fulfillment nor the rejection handler.

@cboden
Copy link
Member

cboden commented Mar 30, 2016

ah. 👍 for 2.4 then

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

4 participants