Skip to content

Conversation

@Jeff-Meadows
Copy link
Contributor

No description provided.

@boxcla
Copy link

boxcla commented Sep 7, 2016

Verified that @Jeff-Meadows has signed the CLA. Thanks for the pull request!

@jmoldow
Copy link
Contributor

jmoldow commented Sep 7, 2016

👍

@Jeff-Meadows Jeff-Meadows force-pushed the fix_logging_network_downloads branch from 893f81f to 1a488f0 Compare September 8, 2016 02:55
@Jeff-Meadows Jeff-Meadows merged commit a918ccf into box:master Sep 8, 2016
@Jeff-Meadows Jeff-Meadows deleted the fix_logging_network_downloads branch September 8, 2016 06:04
jmoldow added a commit that referenced this pull request Sep 22, 2016
Move response logging to a new `LoggingNetworkResponse` class.
This allows us to decide whether to log the response body, based
on whether the caller reads or streams the content. This fixes a
bug introduced by Pull Request #166, where the `LoggingNetwork`
would always omit logging the response body.

Add a `network_response_constructor` property to
`DefaultNetwork`, to make it easier for subclasses to return
their own `NetworkResponse` subclasses. Use this in
`LoggingNetwork`, to return `LoggingNetworkResponse` instances.
Also add it as an optional property on the `Network` interface
class.

In `network_interface.py`, switch some usages of
`@abc.abstractmethod` to `@abc.abstractproperty`. This doesn't
matter on recent versions of Python 3, but there is a difference
between the two on Python 2, plus it makes the interface
clearer. This is a backwards-compatible change, because the SDK
uses these as properties, so any custom implementations would've
needed to implement these with `property` anyway.

Update the logging format strings in `LoggingNetwork`, to make
them more informative and also easier to override (by using
keyword format placeholders instead of positional ones). This is
a breaking change for clients that were overriding these class
attributes.

Add logging for request exceptions in `LoggingNetwork`.

Use the `LoggingNetwork` during functional tests.

Switch some stateful mock fixtures from session scope to
function scope.
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