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

Fix py2->3 string issues #675

Closed

Conversation

foozlevazquez
Copy link

  • Handle exceptions with binary string content. Added test.
  • Handle api_requests with binary content correctly in Python 3. Added test.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 21, 2015
@foozlevazquez
Copy link
Author

See #671 for the full gory history.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 99.98% when pulling 9066b7e on foozlevazquez:dev-fix-py3-str-issues into 56f60bb on GoogleCloudPlatform:master.

@foozlevazquez
Copy link
Author

@dhermes Can you help me understand why this is claiming no coverage, when it is being tested by these tests with and without binary strings? I'm hoping understanding that one will help me understand the other coverage failure. Thanks.

@dhermes
Copy link
Contributor

dhermes commented Feb 21, 2015

From https://travis-ci.org/GoogleCloudPlatform/gcloud-python/builds/51608518#L383

gcloud.datastore.transaction                 32      0      4      0   100%   
gcloud.exceptions                            78      1     14      1    98%   175
gcloud.storage                               38      0     12      0   100%   
gcloud.storage._helpers                      60      0     12      0   100%   
gcloud.storage._implicit_environ              5      0      0      0   100%   
gcloud.storage.acl                          152      0     38      0   100%   
gcloud.storage.blob                         153      0     28      0   100%   
gcloud.storage.bucket                       180      0     48      0   100%   
gcloud.storage.connection                    68      1     18      1    98%   239

It looks like you've got 2 branch misses.


The line numbers are given, so hopefully they are easy to find. Typically a branch miss like this means in code like

if foo == bar:
    baz()

foo is always equal to bar. So you need to add a test for the "other" branch where foo != bar.


You can see this report by running tox -e cover locally. Sometimes a branch miss doesn't get the line reported, and in this case I generate the HTML report

coverage html
ls htmlcov/

@@ -171,6 +171,9 @@ def make_exception(response, content, use_json=True):
:rtype: instance of :class:`GCloudError`, or a concrete subclass.
:returns: Exception specific to the error response.
"""
if not hasattr(content, 'keys') and not isinstance(content, str):

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Feb 21, 2015

It's unclear which of these fixes were needed.

There are only two uses of http.request in our code:

$ git log -1 --pretty=%H  # Current HEAD
56f60bb94b313c397f5aeadee1f0e1f6e5b5290c
$ git grep -n 'http\.request' | egrep -v test | egrep -v -e '^_gcloud_vendor'
gcloud/datastore/connection.py:80:        headers, content = self.http.request(
gcloud/storage/connection.py:169:        return self.http.request(uri=url, method=method, headers=headers,

The auth issue should be fixed upstream in oauth2client.

The only change here is one in storage.Connection but it doesn't seem to address the headers.

WDYT? How did you determine these fixes were needed? (FWIW the isinstance(content, six.string_types) is needed.)

@dhermes
Copy link
Contributor

dhermes commented Mar 9, 2015

I'm closing this out since it's being fixed upstream in httplib2 and oauth2client:
googleapis/oauth2client#104
googleapis/oauth2client#136
jcgregorio/httplib2#291
jcgregorio/httplib2#296

@dhermes dhermes closed this Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants