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

Document Promise #68

Merged
merged 1 commit into from
Jan 6, 2016
Merged

Document Promise #68

merged 1 commit into from
Jan 6, 2016

Conversation

ddeboer
Copy link
Contributor

@ddeboer ddeboer commented Jan 3, 2016

No description provided.

@ddeboer ddeboer changed the title Add Guzzle 6 adapter docs Document Promise Jan 3, 2016
function (ResponseInterface $response) {
echo 'Yay, we have a shiny new response!';

return $response;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the point of returning the response here? that will do nothin i think. maybe we should find a better example what to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

like updating a file or database with the contents of a json response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know, really. I copied this from the tutorial. 😉

Copy link
Member

Choose a reason for hiding this comment

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

No idea either.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should avoid duplication between doc. keep the tutorial minimal, or use a partial if the same example really makes sense (or just link)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

also, some redundancy in a tutorial chapter is ok - the references serve a different purpose and a tutorial without code examples and tons of links is not helpful. ideally the tutorial will "tell a story". it could mention working with promise as a side note as a tutorial on promise would be a large topic of its own...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True: the tutorial should be a kind of story that is easy to follow. Too many links will distract too much and disrupt the flow.

Copy link
Member

Choose a reason for hiding this comment

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

Well, duplication is bad from two points of view: content and maintenance. The former is arguable: for the sake of completeness, it might be good. If we can utilize partials to avoid maintenance of duplication, that's at least a half win.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. but can we turn this into a minimal useful example? like write the response body into a file? returning something makes no sense at all.

@dbu
Copy link
Contributor

dbu commented Jan 3, 2016

great, thanks for starting this!


The ``$promise`` that is returned implements ``Http\Promise\Promise``. At this
point in time, the response is not known yet. You can be polite and wait for
that response to arrive::
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should mention that you could do several async requests in parallel and then wait for each (allowing the other requests to resolve while we wait)

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this happening in the batch client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, as that is usually the idea of doing asynchronous requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s document this in a separate Batch client chapter.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. we can add a note in this chapter when we add the batch client chapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #73.

@ddeboer ddeboer mentioned this pull request Jan 6, 2016
2 tasks
dbu added a commit that referenced this pull request Jan 6, 2016
@dbu dbu merged commit 7859c1e into master Jan 6, 2016
@dbu dbu deleted the promise branch January 6, 2016 08:17
@dbu
Copy link
Contributor

dbu commented Jan 6, 2016

great, thanks!
@joelwurtz if you can write that blogpost, please do and do a PR to link it from this doc chapter.

// Write status code to some log file
file_put_contents('responses.log', $response->getStatusCode() . "\n", FILE_APPEND);

return $response;
Copy link
Contributor

Choose a reason for hiding this comment

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

ups, already merged, but: @joelwurtz does returning the response make any sense here or should we just file_put_contents for the example? i assume the thing resolving the promise is not handling any return values?

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