Skip to content

Conversation

@ddeboer
Copy link
Contributor

@ddeboer ddeboer commented Jan 3, 2016

No description provided.

@ddeboer
Copy link
Contributor Author

ddeboer commented Jan 3, 2016

Oops wait, I forgot to add an installation section. :lol:

Copy link
Contributor

Choose a reason for hiding this comment

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

should we promote this or use the message factory?

Copy link
Member

Choose a reason for hiding this comment

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

I think it rather matters in a library, then an application. I would keep both and state that message factory should be used in a library.

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 wanted to keep things as easy as possible for new users.

And because guzzle6-adapter requires guzzle which requires guzzle/psr7, I thought it made sense to just use Guzzle’s PSR-7 Request object here.

Added a reference to the message factory doc chapter.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Wouldn't it end up too much duplicated documentation if we have this in each client documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s a good argument, too. Let’s try to minimize duplication where possible.

Copy link
Member

Choose a reason for hiding this comment

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

I think only the configuration can be different for clients. So every other usage should be the same hence the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s true, but it makes sense to have complete example per adapter. Later we can extract this duplicated bit and make it an include (long live Sphinx 😉).

Copy link
Member

Choose a reason for hiding this comment

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

Cool.

@ddeboer ddeboer force-pushed the guzzle6-doc branch 3 times, most recently from 9fd919c to 10c168a Compare January 4, 2016 07:39
@ddeboer
Copy link
Contributor Author

ddeboer commented Jan 4, 2016

Installation notes added. Now ready for merging.

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 prefer this bottom section over documentation sprinkled with note and warning boxes, as they can be very distracting.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍
we might want to move this into a partial that we include right away, as this page will be used as template for other adapter / client documentations.

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 do that in subsequent PRs for the adapters.

@ddeboer
Copy link
Contributor Author

ddeboer commented Jan 4, 2016

Updated according to feedback.

dbu added a commit that referenced this pull request Jan 5, 2016
@dbu dbu merged commit fa9580b into master Jan 5, 2016
@dbu dbu deleted the guzzle6-doc branch January 5, 2016 07:22
@dbu
Copy link
Contributor

dbu commented Jan 5, 2016

thanks a lot!

@dbu dbu mentioned this pull request Jan 5, 2016
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