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

Add config for enable FlexibleHttpClient and HttpMethodsClient #83

Merged
merged 4 commits into from
Jun 30, 2016

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jun 29, 2016

This will address part of #70

With config options you wrap your client in FlexibleHttpClient or HttpMethodsClient. Please give input on the config names. I want them to be descriptive but at the same time also reflect the class we use for wrapping.

A descriptive config name would be "emulate_async_client" but that is very far from the name FlexibleHttpClient. I guess you understand my point. Any suggestions are welcome.

*/

if ($arguments['flexible_client'] && $arguments['add_http_methods']) {
throw new \LogicException('A http client can\'t be decorated with both FlexibleHttpClient and HttpMethodsClient. Only one of the following options can be true. ("flexible_client", "add_http_methods")');
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be validated in the Configuration class. you can add a validate() section (or was it validation()?) on the level that has the two options.

@dbu
Copy link
Collaborator

dbu commented Jun 30, 2016

the name flexible_client seems right to me. i would call the other http_methods_client.

@Nyholm
Copy link
Member Author

Nyholm commented Jun 30, 2016

Thank you for your review. I've update the PR according to your suggestions

@dbu
Copy link
Collaborator

dbu commented Jun 30, 2016

thanks a lot @Nyholm !
created php-http/documentation#107 for the documentation.

@Nyholm
Copy link
Member Author

Nyholm commented Jun 30, 2016

Thank you for merging.

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.

None yet

3 participants