-
Notifications
You must be signed in to change notification settings - Fork 268
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
Major touchup #11
Major touchup #11
Conversation
omares
commented
Aug 13, 2014
- Added docblocks
- Added namespace
- Improved composer configuration
- Streamlined the API (BC Breaks).
- Refactored code for more readability
- Added adapters. Guzzle as client is now interchangeable.
- Added request and response filters. Users can now modify the Symfony requests and responses to their liking.
public function __construct(Client $client) | ||
{ | ||
$this->client = $client; | ||
$this->messageFactory = $this->messageFactory = new MessageFactory(); |
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.
seems redundant
Btw, the project should be added to the https://packagist.org/ list for easier installation. :) |
Having a different Factory class for each adapter seems a bit overkill to me. Would it not be better to use something similar like the Laravel connection manager? A single class that would create proxy objects based on the default adapter or by selecting a different adapter. Something like this:
This is similar to what I'm using from Lusitanian/PHPoAuthLib:
|
Old style API
@jenssegers Yeah i would apply the Abstract Factory pattern (A Main Factory uses the various other Factories to create the one you want) but atm there is only one Adapter and so there is no need. Anyway the GuzzleFactory should stay because the MainFactory would need it to for creation. Btw if we are okay with the current api and feature set ill start creating unit tests. |
I just want it to be as easy as possible for developers to use this proxy. Creating too many classes might make it a bit too difficult to get started. |
Thats why i wouldnt add another factory ;) |
I merged the PR, let me know what you think. For now, I included Guzzle in the composer file. Because it seemed a bit silly to suggest a package which it needs to work properly. |
Cool, thanks for the merge! :) Working on some unittests right now. Here is a my feedback regarding your changes:
|
And overall i would suggest following the PSR-2 Standard regarding formatting, naming, etc. Makes maintenance easier when you can follow a common standard. |
|
|