Skip to content

Allow for a candidate to have a Closure as a class parameter #66

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

Merged
merged 3 commits into from
Jul 5, 2016

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jul 2, 2016

This PR will allow you to return a closure as a class parameter on the candidate. The good thing about this is that someone can write their own strategy that emulates a async client. An example of such strategy is seen in #65.

This will increase the extendibility in the discovery mechanism.

@dbu
Copy link
Contributor

dbu commented Jul 3, 2016

squash and merge?

@Nyholm
Copy link
Member Author

Nyholm commented Jul 5, 2016

I rebased the PR and added implements Exception.

FYI. I do not want the ClassDiscovery to instantiate the class because the HttpClientDiscovery may want to give a parameter to the closure in the future. Im not sure that will ever happen, but at least this will give room for that possibility.

@dbu dbu merged commit 2ed6663 into php-http:master Jul 5, 2016
@dbu
Copy link
Contributor

dbu commented Jul 5, 2016

thanks!

@Nyholm
Copy link
Member Author

Nyholm commented Jul 5, 2016

Thank you for merging.

@Nyholm Nyholm deleted the closure branch July 5, 2016 07:50
/**
* Get an instance of the $class.
*
* @param string|\Closure $class A FQN of a class or a closure that instantiate the class.
Copy link
Member

Choose a reason for hiding this comment

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

FQCN

@sagikazarmark
Copy link
Member

Added a few comments. Nice work. 👍

Nyholm added a commit to Nyholm/discovery that referenced this pull request Jul 7, 2016
dbu pushed a commit that referenced this pull request Jul 7, 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.

3 participants