-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update discovery documentation #105
Conversation
99e898e
to
816142f
Compare
|
||
The package supports multiple discovery strategies and comes with two out-of-the-box: | ||
|
||
- A built-in strategy with official HTTPlug components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A built-in strategy supporting the HTTPlug adapters for the most common HTTP clients Guzzle 6 and Diactoros." (i think being specific in this place will be helpful, even at the risk that the list could grow out of sync.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is needed. It just creates an other list to maintain. I suggest no change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @dbu rather meant the most common PSR7 implementations in this case. I don't think it's wrong, actually it's marketing. 😛
816142f
to
f725525
Compare
@@ -62,8 +87,8 @@ Common Errors | |||
Puli Factory is not available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still a thing @Nyholm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But only as "previous exception" See comment: php-http/discovery#59 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, should we reflect that in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good as it is.
But we should add docs about the "DiscoveryFailedException" and maybe rewrite the "NotFoundException" a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to get this merged ASAP. I created #116
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good!
Someone pls do a final review |
thanks for keeping the doc in sync ! once there is a stable 1.0 version, we should add versionadded blocks when we add new features, to make the doc useful and understandable for existing users. with pre-1.0 versions we should not bother or we clutter docs with too much information. |
I added only one: the NotFoundException deprecation |
No description provided.