support eu api domain #20
Conversation
aviau
force-pushed
the
aviau/api-domains
branch
3 times, most recently
from
95a0620 to
a9c198a
Compare
|
|
||
| api_domain = api_domain or _API_DOMAIN_DEFAULT | ||
| if api_domain not in _ALLOWED_API_DOMAINS: | ||
| raise Exception( |
There was a problem hiding this comment.
I would raise ValueError instead of a generic Exception
There was a problem hiding this comment.
Does it matter? Is it an exception that will be caught?
There was a problem hiding this comment.
It matters for the tests. The test actually catches a generic Exception and will fail to fail if the exception is raised by a coding error, whereas with a ValueError code errors can be caught. Our API users can also better understand that the exception was raised because of a value that was provided by them instead of being an error that we caused in our code.
There was a problem hiding this comment.
But the tests match the exact exception message:
with pytest.raises(Exception, match="Invalid API domain"):There was a problem hiding this comment.
I would argue that setting a specific exception type will increase our API surface? Meaning that our users might start writing code that handles this exception differently. Are we sure we want to support that? Are we willing to commit to it forever?
aviau
force-pushed
the
aviau/api-domains
branch
from
a9c198a to
2dd72e5
Compare
aviau
force-pushed
the
aviau/api-domains
branch
3 times, most recently
from
24f92cb to
1ea6433
Compare
aviau
force-pushed
the
aviau/api-domains
branch
from
1ea6433 to
4a2e8c0
Compare
No description provided.