ash-jc-allen / laravel-exchange-rates

A Laravel wrapper package for interacting with the exchangeratesapi.io API.
MIT License
449 stars 49 forks source link

Response abstraction #126

Closed spotwilliams closed 1 year ago

spotwilliams commented 1 year ago

Added AshAllenDesign\LaravelExchangeRates\Interfaces\ResponseContract interface that represent an exchange response API.

By implementing this the AshAllenDesign\LaravelExchangeRates\Drivers\Support\SharedDriverLogicHandler doesn't need to know about internal responses specifications (the keys of the response array, the shape of the body, etc), and request the rates from the object that implements ResponseContract.

ash-jc-allen commented 1 year ago

Hey @spotwilliams, I love this! I'm super busy this week, but I'll try and take a proper look at this ASAP 😄

ash-jc-allen commented 1 year ago

Hey @spotwilliams! I've just got a quick question about the get method in Response objects. I was just wondering what the ?? collect($this->rawResponse)->flatten()->get($key) part was for?

From running the tests, it seems like this method will work with just:

public function get(string $key): mixed
{
    return $this->rawResponse[$key];
}

So I was just wondering if I'm missing anything and if the null-coalescing is needed? 🙂

spotwilliams commented 1 year ago

Hi @ash-jc-allen, yes let me explain:

return $this->rawResponse[$key] ?? collect($this->rawResponse)->flatten()->get($key);

Here's what I'm trying to do is be ready to any changes in the response structure, so if the rate is not directly in the raw response, maybe can be in deeper levels.

My thoughts was to have a code that could be ready to possible changes and be able to work after those. But, now that I think it feels like an overhead approach.

ash-jc-allen commented 1 year ago

@spotwilliams Ahh, that makes sense! I thought it might have been something like that, but wasn't too sure.

In theory, I'd like to think there wouldn't be any changes to the API response structure (but I know that's definitely not always the case haha!). So I'd be tempted to remove the null-coalescing and just use the array access. We can always add that back in the future though if we do need to react to any API response changes.

What are your thoughts on this? 🙂

spotwilliams commented 1 year ago

Agree with you @ash-jc-allen , let's keep it simple. 🚀