Rewording of README and docblock #35

kelunik · 2016-12-31T20:12:11Z

This PR supercedes #34. It additionally formats @param as in async-interop/event-loop.

bwoebi · 2016-12-31T20:19:19Z

src/Promise.php

-     * Warning: If you use type declarations for `$value`, be sure to make them accept `null` in case of failures.
-     *
-     * @param callable(\Throwable|\Exception|null $exception, mixed $value) $onResolved Callback to be executed.
+     * @param callable(mixed $reason, mixed $value) @onResolved `$reason` shall be `null` on success, `$value` shall be


Why did you remove the type of reason? I want to see at the first look as callee that I'll only ever be passed something I can throw.

See old PR, I asked that, too.

Well, then please revert that. \cc @krakjoe

I reiterate, please revert this.

@bwoebi Discuss this with @krakjoe.

it's clear to me now bob ~@krakjoe

Please revert it now ;o)

bwoebi · 2017-01-05T17:02:06Z

README.md

@@ -37,7 +37,7 @@ A `Promise` MUST be in one of three states: `pending`, `succeeded`, `failed`.

 ## Consumption

-A `Promise` MUST implement `Interop\Async\Promise` and thus provide a `when()` method to access its current or eventual value or reason.
+A `Promise` MUST implement `Interop\Async\Promise` and thus provide a `when()` method to access its value or reason.


I'd retain the "current or future" here as there is no other temporal indication here, currently suggesting that the current value/reason could be meant.

I think it's fine as is combined with the rest of the spec.

bwoebi · 2017-01-05T17:04:43Z

README.md


-All callbacks registered before the resolution MUST be executed in the order they were registered. Callbacks registered after the resolution MUST be executed immediately. If one of the callbacks throws an `Exception` or `Throwable`, it MUST be forwarded to `Async\Interop\Promise\ErrorHandler::notify`. The `Promise` implementation MUST then continue to call the remaining callbacks with the original parameters.
+All callbacks registered before the resolution MUST be executed in the order they were registered. Callbacks registered after the resolution MUST be executed immediately. If one of the callbacks throws an `Exception` or `Throwable`, it MUST be forwarded to `Interop\Async\Promise\ErrorHandler::notify`. The `Promise` MUST then continue to call the remaining callbacks with the original parameters.


AsyncInterop now.

Sure, wasn't rebased yet.

bwoebi · 2017-01-05T17:05:51Z

README.md

-Any implementation MUST at least provide these two parameters. The implementation MAY extend the `Promise` interface with additional parameters passed to the callback. Further arguments to `when()` MUST have default values, so `when()` can always be called with only one argument. `when()` MAY NOT return a value. `when()` MUST NOT throw exceptions bubbling up from a callback invocation.
-
-> **NOTE:** The signature doesn't specify a type for `$error`. This is due to the new `Throwable` interface introduced in PHP 7. As this specification is PHP 5 compatible, we can use neither `Throwable` nor `Exception`.
+The implementation MAY extend `Promise::when` with additional parameters passed to the callback. Further arguments to `Promise::when()` MUST have default values, so `Promise::when()` can always be called with only one argument. `Promise::when()` MAY NOT return a value. `Promise::when()` MUST NOT throw exceptions bubbling up from a callback invocation.


the first Promise::when is lacking a trailing parenthesis pair.

Further arguments to Promise::when() MUST have default values, so Promise::when() can always be called with only one argument. Can be removed altogether IMHO, as that's anyway covered by interface semantics.

Added parens.

…ert callable docs

bwoebi · 2017-01-05T20:34:15Z

README.md

@@ -56,20 +56,18 @@ interface Promise
     *
     * If the promise is already resolved, the callback MUST be executed immediately.
     *
-     * @param callable(mixed $reason, mixed $value) @onResolved `$reason` shall be `null` on success, `$value` shall be
-     *     `null` on failure.
+     * @param callable(\Throwable|\Exception|null $exception, mixed $value) @onResolved `$reason` shall be `null` on


I would leave mixed out… just $value.

bwoebi · 2017-01-05T20:35:09Z

README.md

     *
-     * @return void
+     * @return mixed Return type and value are unspecified.


In that case one does just omit @return from docblock. State it in the text, but this is not appropriate in docblock.

It's the right way if we want that. Omitting is equivalent to @return void, see https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#715-return

(IMHO void is what we want to mean, according to my interpretation of void within LSP… but fine. … not going to insist here.)

Your interpretation is not what void currently is in PHP.

Meh, PHP is only supporting a subset of what should be possible due to LSP anyways... As said, I don't insist.

kelunik · 2017-01-06T19:57:50Z

@trowski @jsor Can we go ahead and merge this one?

kelunik force-pushed the krakjoe-rewording branch from 957e6b0 to d0258b3 Compare December 31, 2016 20:15

bwoebi reviewed Dec 31, 2016

View reviewed changes

bwoebi reviewed Jan 5, 2017

View reviewed changes

krakjoe and others added 2 commits January 5, 2017 19:02

rewording of readme and docblock

7b3b87d

Adjust PHPDoc to match the rest of async-interop

ac187c1

kelunik force-pushed the krakjoe-rewording branch from d0258b3 to ac187c1 Compare January 5, 2017 18:02

kelunik added 4 commits January 5, 2017 19:04

Fix rebase fail

656e897

Improve wording for when to call when callbacks

4cefae5

Add missing parens

1c4c2d9

Allow implementations to return values by leaving it unspecified, rev…

a55d386

…ert callable docs

bwoebi reviewed Jan 5, 2017

View reviewed changes

kelunik mentioned this pull request Jan 5, 2017

rewording of readme and docblock #34

Closed

Remove unnecessary mixed from declaration

72a08e5

bwoebi approved these changes Jan 5, 2017

View reviewed changes

kelunik requested review from jsor and trowski January 6, 2017 19:58

jsor approved these changes Jan 6, 2017

View reviewed changes

WyriHaximus approved these changes Jan 6, 2017

View reviewed changes

bwoebi merged commit 511ae43 into master Jan 6, 2017

kelunik deleted the krakjoe-rewording branch January 20, 2017 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewording of README and docblock #35

Rewording of README and docblock #35

kelunik commented Dec 31, 2016

bwoebi Dec 31, 2016

kelunik Dec 31, 2016

bwoebi Dec 31, 2016

bwoebi Jan 5, 2017

kelunik Jan 5, 2017

bwoebi Jan 5, 2017

bwoebi Jan 5, 2017

kelunik Jan 5, 2017

bwoebi Jan 5, 2017

kelunik Jan 5, 2017

bwoebi Jan 5, 2017

bwoebi Jan 5, 2017 •

edited

Loading

kelunik Jan 5, 2017

bwoebi Jan 5, 2017

bwoebi Jan 5, 2017

kelunik Jan 5, 2017

bwoebi Jan 5, 2017

kelunik Jan 5, 2017

bwoebi Jan 5, 2017

kelunik commented Jan 6, 2017


		All callbacks registered before the resolution MUST be executed in the order they were registered. Callbacks registered after the resolution MUST be executed immediately. If one of the callbacks throws an `Exception` or `Throwable`, it MUST be forwarded to `Async\Interop\Promise\ErrorHandler::notify`. The `Promise` implementation MUST then continue to call the remaining callbacks with the original parameters.
		All callbacks registered before the resolution MUST be executed in the order they were registered. Callbacks registered after the resolution MUST be executed immediately. If one of the callbacks throws an `Exception` or `Throwable`, it MUST be forwarded to `Interop\Async\Promise\ErrorHandler::notify`. The `Promise` MUST then continue to call the remaining callbacks with the original parameters.

Rewording of README and docblock #35

Rewording of README and docblock #35

Conversation

kelunik commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwoebi Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik commented Jan 6, 2017

bwoebi Jan 5, 2017 •

edited

Loading