Update Swap #307
Update Swap #307
Conversation
|
I wonder if we should rather have a static factory for ExchangeRateProvider instead of the current solution. @florianv your feedback is very welcome. |
|
Thanks @sagikazarmark for taking initiative on that! This looks ok to me, nothing to add 👍 |
|
Thanks for the feedback @florianv |
| @@ -42,6 +44,9 @@ | |||
| "florianv/swap": "Exchange rates library for PHP", | |||
| "psr/cache-implementation": "Used for Currency caching" | |||
| }, | |||
| "conflict": { | |||
| "florianv/swap": "<3.0" | |||
There was a problem hiding this comment.
Is it really needed? It's impossible to install this package with florianv/swap<3.0.
There was a problem hiding this comment.
Hm indeed, it's an optional dependency after all.
There was a problem hiding this comment.
Haven't noticed it's in require-dev, so in fact it is possible to install it with lower Swap version, but as you said, it's optional dependency so there is no sense to restrict it.
| */ | ||
| private $swap; | ||
|
|
||
| /** | ||
| * @param SwapInterface $swap | ||
| * @param Swap|ExchangeRateProvider $exchange |
There was a problem hiding this comment.
If it's SwapExchange that uses Swap, shouldn't we make ExchangerExchange that uses ExchangeRateProvider?
There was a problem hiding this comment.
Well, we would basically duplicate Swap with an exchange like that. Swap is a convenience wrapper around ExchangeRateProvider. However this exchange does not work without Swap, so I am not sure if we should just use Swap or not. It's just new SwapExchange(new Swap(/**/)) after all.
| } | ||
|
|
||
| if ($exchange instanceof ExchangeRateProvider) { | ||
| $exchange = new Swap($exchange); |
There was a problem hiding this comment.
Anyway, what about just not accepting ExchangeRateProvider here, it seems easy to make Swap instance out of it and doing that will reduce complexity.
| $exchange = new Swap($exchange); | ||
| } | ||
|
|
||
| $this->swap = $exchange; |
Applied fixes from StyleCI
Closes #298