Skip to content
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

Merged

Conversation

macisamuele
Copy link
Collaborator

The main objective of this PR is to provide support to ConnectionErrors as is currently provided for TimeoutErrors.
This aims to address also @bplotnick suggestions in PR #346.

Details of this PR code changes

  1. Integration tests base class is no longer TestServerRequestsClient. This is intended done in order to move the base integration tests suite into bravado.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 future
  2. changed the way that exceptions are raised:
  • Timeout classes are not saved as future attribute. This is done as futures are created for every request the timeout class was being created every time anyway and because it could be possible that classes are not compatible each others (ie. class 1 and class 2 requires different arguments to be allocated and the new class extends both of them). This was not an issue until now as timeout_errors contained a single item, but with connection errors (at least with fido client) we have more that one exception class
  • Re-raising an exception uses an hack to make sure that we'll be able to instantiate the error class without changing the properties (attributes and methods) that the original exception class provides

@macisamuele macisamuele requested a review from sjaensch June 25, 2018 07:32
@coveralls
Copy link

coveralls commented Jun 25, 2018

Coverage Status

Coverage increased (+0.04%) to 98.172% when pulling f1ea220 on macisamuele:maci-add-connection-error-support into 195c0ea on Yelp:master.

Copy link
Contributor

@sjaensch sjaensch left a 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 ())
Copy link
Contributor

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.

Copy link
Collaborator Author

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)

Copy link
Collaborator Author

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

@macisamuele macisamuele merged commit b4fa537 into Yelp:master Jun 25, 2018
@macisamuele macisamuele deleted the maci-add-connection-error-support branch June 25, 2018 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants