-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add connection error support #377
Add connection error support #377
Conversation
…nd each clients tests class extends from it
…comodate connection errors handling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks good to me! One small change and then 🛳!
), | ||
) | ||
timeout_errors = tuple(self.future.timeout_errors or ()) | ||
connection_errors = tuple(self.future.connection_errors or ()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this code backwards compatible? I think something like
connection_errors = getattr(self.future, 'connection_errors', ())
I don't think it's a problem to use whatever the value of that attribute is if it is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure ... keeping backward compatibility is something to aim for :)
I'm not against using getattr
but I would understand how the change could provide backward incompatible changes (so next time I'll think to it too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about it and well the change is backward compatible as self.future
in an instance of bravado.http_future. FutureAdapter
which defines those attributes.
So even if http clients are not defining them the attribute will be available with an empty tuple as default value
The main objective of this PR is to provide support to
ConnectionError
s as is currently provided forTimeoutError
s.This aims to address also @bplotnick suggestions in PR #346.
Details of this PR code changes
TestServerRequestsClient
. This is intended done in order to move the base integration tests suite intobravado.testing
module so external http clients (ie. bravado-asyncio) could be able to run the same tests that we run for the http clients provided by bravado.NOTE: moving integration tests into
bravado.testing
is not addressed here, a new PR will be provided in the futuretimeout_errors
contained a single item, but with connection errors (at least with fido client) we have more that one exception class