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

Documentation improvement #110

Merged
merged 1 commit into from
Oct 7, 2019
Merged

Documentation improvement #110

merged 1 commit into from
Oct 7, 2019

Conversation

seregazhuk
Copy link
Contributor

This PR provides some suggestion for documentation improvement:

Promise\race().

Returns a promise which is resolved in the same way the first settled promise resolves

Word resolves confused me that race() returns a promise that resolves once the first promise from the set resolves. But actually, it resolves once the first promise settles. If the settled promise resolves the resulting promise resolves with its resolution value, otherwise it resolves with a rejection reason.

Promise\some()

It is not explicitly said if it expects the exact number of promises to be resolved or at least of this number. I've also added that the resolution value of the resulting promise contains resolution values from the promises that were resolved first.

Maybe these suggestions look like salt is salty, but I think that documentation should be as explicit as possible.

@jsor
Copy link
Member

jsor commented Jan 17, 2018

Thanks, that's definitely something which can be improved 👍
The new wording though actually feels more confusing to me. Note, that resolved is not the same as fulfilled. A good explanation of the different adjectives can be found at https://github.com/domenic/promises-unwrapping/blob/master/docs/states-and-fates.md

@seregazhuk
Copy link
Contributor Author

@jsor Thank you for your feedback. Now I see that React\Promise\some() description was perfectly fine 🤦‍♂️ So, I've left improvement only for React\Promise\some().

@@ -447,10 +447,10 @@ if `$promisesOrValues` contains 0 items.
$promise = React\Promise\some(array $promisesOrValues, integer $howMany);
```

Returns a promise that will resolve when `$howMany` of the supplied items in
Returns a promise that will resolve when at least `$howMany` of the supplied items in
Copy link
Member

Choose a reason for hiding this comment

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

at least is wrong here. The output promise fulfills after exactly $howManyfulfill and ignores the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced word resolve with fulfill.

My intention with word at least was to show, that it is enough for $howMany promises to be fulfilled. For example, when we have 3 pending promises, it is enough for 2 of them to be fulfilled. Not exactly 2 should be fulfilled, and 1 should fail.

@seregazhuk
Copy link
Contributor Author

I'm still a bit confused about statuses. Documentation says that Deferred::resolve() resolves the promise and Deferred::reject() rejects the promise. But according this doc rejected promise is also resolved:

A promise is resolved if it ... has been fulfilled or rejected

Does this mean that Deferred::resolve() actually fulfills the promise? Or words resolved and fulfilled can sometimes be interchangeable?

@jsor
Copy link
Member

jsor commented Jan 19, 2018

That sentence is wrong and we should probably rework the whole documentation to use the correct adjectives :)

resolved means, the promise's fate is sealed and any subsequent calls to resolve() and reject() have no effect. But that doesn't mean the promise is settled because it might follow another promise (resolve($anotherPromise)) which itself is still pending. Then the promise will settle once $anotherPromise settles.

@seregazhuk
Copy link
Contributor Author

Now I see :) Thanks for clarifying 👍

@WyriHaximus WyriHaximus modified the milestones: v2.7.1, v3.0.0 Jan 7, 2019
@WyriHaximus
Copy link
Member

ping @clue

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@seregazhuk Thanks for your patience, changes LGTM 👍 Can you squash this into a single commit?

@seregazhuk
Copy link
Contributor Author

@clue Done!

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

:shipit:

@clue clue merged commit b02f512 into reactphp:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants