Move API key in request header and change body to json format (#89) #90
Conversation
cougargit
commented
Feb 4, 2025
cougargit
commented
- also add examples for live testing and development scenarios
cougargit
changed the title
cougargit
changed the title
| private DeeplRequestFactoryInterface $deeplRequestFactory; | ||
|
|
||
| private ClientInterface $httpClient; | ||
|
|
||
| private RequestFactoryInterface $requestFactory; | ||
|
|
||
| public function __construct( | ||
| string $apiKey, |
There was a problem hiding this comment.
possibly nullable if you don't have one?
There was a problem hiding this comment.
possibly nullable if you don't have one?
Does it even make sense constructing the object without having an apikey? I don't think so...
There was a problem hiding this comment.
possibly nullable if you don't have one?
Does it even make sense constructing the object without having an apikey? I don't think so...
One option for example is to throw an Exception('No API key given'), but the DeepL API cannot be used without apikey, whether you use the free or the Pro version makes no difference.
What solution is prefered?
There was a problem hiding this comment.
possibly nullable if you don't have one?
Does it even make sense constructing the object without having an apikey? I don't think so...
One option for example is to throw an Exception('No API key given'), but the DeepL API cannot be used without apikey, whether you use the free or the Pro version makes no difference.
What solution is prefered?
The only reason to make the api-key nullable + default-null here (and as the last parameter in the list) is backward compatibility.
If the api-key is not nullable, then this change is not compatible and requires a new major version in case someone creates the client without using the factory.
However, since the code is no longer 100% backwards compatible even with an exception in the case of a missing api key, it will end up with a new major version anyway. And then we can do without it and not make the api key nullable.
Unless you find a way to implement the whole thing in a way that makes it backwards compatible. But since the effort is probably not worth it, I would personally be in favor of: non-nullable and a new major version.
There was a problem hiding this comment.
Unless you find a way to implement the whole thing in a way that makes it backwards compatible. But since the effort is probably not worth it, I would personally be in favor of: non-nullable and a new major version.
I would agree with that "non-nullable and a new major version" to prevent surprises using this lib.
- also add examples for live testing and development scenarios
cougargit
force-pushed
the
moveApiKeyInHeader
branch
from
f9127be to
ee611cc
Compare
