SG-38959 improvements #419
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses improvements related to SG-38959, focusing on upload request handling, URL sanitization, and documentation updates. The changes simplify HTTP error handling, introduce a utility for safe URL logging, and improve the clarity of logging configuration examples.
Key Changes:
- Simplified HTTP error handling in upload requests by removing redundant try-except block
- Added
safe_short_url()utility function to sanitize URLs in logs by removing credentials and truncating long paths - Updated logging documentation with clearer examples showing proper logger configuration
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| shotgun_api3/shotgun.py | Simplified _make_upload_request() error handling, refactored request creation to use modern syntax, added safe_short_url() utility for sanitizing URLs in logs, and included debug code/TODOs for testing |
| tests/test_client.py | Added TODO comments for future test coverage improvements |
| docs/cookbook/usage_tips.rst | Improved logging configuration examples with proper logger setup and consistent string quotes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shotgun_api3/shotgun.py
Outdated
| storage_url = "https://wrong.host.badssl.com/" | ||
|
|
There was a problem hiding this comment.
Debug code is overwriting the storage_url parameter, which will break file uploads to production storage. This code must be removed before merging.
| storage_url = "https://wrong.host.badssl.com/" |
| :param int size: Number of bytes in the data stream. | ||
| :param str storage_url: Target URL for the uploaded file. | ||
| :returns: upload url. | ||
| :returns: upload url - NO! Return the AWS object ID. |
There was a problem hiding this comment.
The return value description is unclear and contains an editorial comment ('NO!'). Update this to clearly document what is actually returned by the function.
| :returns: upload url - NO! Return the AWS object ID. | |
| :returns: The AWS object ID (ETag) for the uploaded part. |
| :rtype: str | ||
| """ | ||
|
|
||
| ## TODO - add unitests for those cases |
There was a problem hiding this comment.
Corrected spelling of 'unitests' to 'unit tests'.
| ## TODO - add unitests for those cases | |
| ## TODO - add unit tests for those cases |
shotgun_api3/shotgun.py
Outdated
| LOG.debug("Completed request to %s" % request.get_method()) | ||
| LOG.debug(f"Completed request to {safe_short_url(storage_url)}") | ||
|
|
||
| # FIXME - why don't we capture SSL errors here? |
There was a problem hiding this comment.
This FIXME indicates incomplete error handling logic. SSL errors should either be explicitly handled or the comment should be addressed before merging.
Also unify the loop and attempts code accross all methods
Also unify codes on all methods
1 participant
WIP