link0 / bunq

API client in PHP for bunq
MIT License
8 stars 13 forks source link

Guzzle proxy support #29

Open holtkamp opened 7 years ago

holtkamp commented 7 years ago

As suggested in https://github.com/link0/bunq/issues/27

dennisdegreef commented 7 years ago

This looks awesome! Thanks for this!

One question though, why are docblocks being removed in this PR?

holtkamp commented 7 years ago

One question though, why are docblocks being removed in this PR?

mmm, in my opinion they should only be used when adding information... Since PHP 7.1 proper return types are supported, so no PHPDoc is required anymore...

Also see this thread: https://twitter.com/akrabat/status/878345065696985088

dennisdegreef commented 7 years ago

Interesting. I tend to agree with the provided point. On the other hand, should we then allow multiple types for a proxy? Namely, a string or an array? Or should we choose one over the other?

holtkamp commented 7 years ago

On the other hand, should we then allow multiple types for a proxy? Namely, a string or an array? Or should we choose one over the other?

First I went with array, since it provides more options for configuration. But eventually, to be able to use a static IP, a plain HTTP proxy suffices => so I went with the string.

Then probably only the string suffices.... But hey, why limit it? The current approach allows for best of both worlds and it is up to the user what info is handed over to Guzzle...

dennisdegreef commented 7 years ago

So can we typehint on an array (also allowing a single entry) and validate the strings in that array? As long as we are pushing towards stricter typehinted code, might as well only allow a single type as a parameter.

holtkamp commented 7 years ago

So can we typehint on an array

Sure, but why restrict the options? We can just follow the Guzzle interface for it right? string + array http://docs.guzzlephp.org/en/stable/request-options.html#proxy

holtkamp commented 7 years ago

shameless bump on this? 😄

holtkamp commented 7 years ago

@dennisdegreef another bump on this?