Skip to content

SG-13264 Update sgsix, fix httplib2 import - #220

Merged
willis102 merged 11 commits into
masterfrom
SG-13264-update_sgsix
Dec 11, 2019
Merged

SG-13264 Update sgsix, fix httplib2 import#220
willis102 merged 11 commits into
masterfrom
SG-13264-update_sgsix

Conversation

@willis102

@willis102 willis102 commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

Update sgsix module to include additional functionality added during port of tk-core.

Update the httplib2 import procedure to ensure that members not exposed in __all__ are still available as expected.

Comment thread shotgun_api3/lib/sgsix.py Outdated
@coveralls

coveralls commented Dec 6, 2019

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1319

  • 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 69.737%

Changes Missing Coverage Covered Lines Changed/Added Lines %
shotgun_api3/lib/sgsix.py 5 6 83.33%
Totals Coverage Status
Change from base Build 1308: 0.04%
Covered Lines: 6897
Relevant Lines: 9890

💛 - Coveralls

@willis102
willis102 requested a review from jfboismenu December 6, 2019 21:08
@willis102

Copy link
Copy Markdown
Contributor Author

@jfboismenu I requested your review here - I know CI is failing, but I believe it's unrelated to these changes. I'm investigating that but figured I'd get your eyes on this PR in the meantime.

@jfboismenu jfboismenu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please add a small test demonstrating that everything that should be there is there after the change. Thanks.

Comment thread shotgun_api3/lib/httplib2/__init__.py Outdated
del __name

# Add ssl_error_classes to __all__
__all__.append('ssl_error_classes')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use double-quotes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂ ✅

from .python2 import socks # ensure include in namespace
from .python2 import SSLHandshakeError # TODO: shouldn't rely on this. not public
ssl_error_classes = (SSLHandshakeError,)
# Generate ssl_error_classes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test demonstrating that nothing is missing?

@willis102 willis102 Dec 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test. Let me know if you'd like to see anything else covered, or if you think I should move it to a different location (I added it in test_api, since I didn't want to add a whole new file for just one test.)

@willis102
willis102 requested a review from jfboismenu December 9, 2019 20:32

@jfboismenu jfboismenu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@willis102
willis102 merged commit 3ec86fe into master Dec 11, 2019
@willis102
willis102 deleted the SG-13264-update_sgsix branch December 11, 2019 17:42
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