Skip to content

Conversation

@ruichen-wang
Copy link

@ruichen-wang ruichen-wang commented Dec 5, 2019

Feel free to leave comments for the changes in my branch here.

I have to add a test_credentials.json to make tests run.

And I am only focusing on the clients we are using due to some clients are for legacy APIs which may already get dropped by HubSpot. (Weird errors keep popping up during the test.)

Those unused and potential legacy wrapper clients just get deleted in my branch.

All left tests passed under python 2.7 and 3.7.

Now the portal will be using the last commit of my working branch in CbitLabs. I do not think this branch actually needs to be merged to the master branch, since we may want to save a copy of the original full Hapipy dependency for potential later reference right?

@pjsg
Copy link

pjsg commented Dec 5, 2019

I think that once we release the portal code to master that uses this branch, we would want to merge this to master as well. As you note, doing that doesn't really change anything except make it easier for future revisions.

@pjsg
Copy link

pjsg commented Dec 5, 2019

Which branch from the upstream repo did you use as the base for this branch? I want to just look at our changes (which should be small)

@ruichen-wang
Copy link
Author

ruichen-wang commented Dec 5, 2019

Which branch from the upstream repo did you use as the base for this branch? I want to just look at our changes (which should be small)

I build my branch on top of the master branch of this CbitLabs repo. If you want to see our customized changes, you could check the commit history of the master branch. https://github.com/CBitLabs/hapipy/commits/master @pjsg

@ruichen-wang
Copy link
Author

Before, this commit is being used in the portal. 8f2859e @pjsg

@pjsg
Copy link

pjsg commented Dec 5, 2019

The upstream master should have been merged in first (from https://github.com/HubSpot/hapipy)

@ruichen-wang
Copy link
Author

ruichen-wang commented Dec 5, 2019

The upstream master should have been merged in first (from https://github.com/HubSpot/hapipy)

We stop rebasing our master branch (CbitLabs) on top of the master branch(HupSpot) since June 19, 2015 and we just keep doing our own changes and version bumps while there are new but less important changes in the upstream at the sam time.

Do you think it is necessary to accept upstream's updates again now? If so, I would rebase our master branch first and backmerge master branch for this working branch.

@ruichen-wang ruichen-wang requested review from cliebBS and tinyx December 5, 2019 21:26
@ruichen-wang
Copy link
Author

Deployed codes to iso1 instance. In BitSight Technologies Corporate company page, I made an EVA request and succeeded in receiving 2 follow up emails from eva@bitsight.com.

Feel free to check the list here. https://app.hubspot.com/contacts/277648/contact/139430878/

@ruichen-wang ruichen-wang requested a review from pjsg December 9, 2019 20:50
raise HapiTimeout(None, request, traceback.format_exc())

encoding = [i[1] for i in result.getheaders() if i[0] == 'content-encoding']
encoding = [i[1] for i in result.getheaders() if i[0].lower() == 'content-encoding']
Copy link
Author

Choose a reason for hiding this comment

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

('content-encoding', 'gzip') in python 2, ('Content-Encoding', 'gzip') in python 3. Convert it to lowercase for python2/3 compatibility here.

return self.__unicode__().encode('ascii', 'replace')


def __unicode__(self):
Copy link
Author

@ruichen-wang ruichen-wang Dec 10, 2019

Choose a reason for hiding this comment

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

There is no builtin __unicode__ function in python3. @python_2_unicode_compatible is for python2 compatibility. It has no effect in python3.
But it will encode unicode string with utf8 as returned value of function __str__ in python2, which has the same behavior as before. Link for reference: http://python-future.org/compatible_idioms.html#custom-str-methods

def test_unicode_error(self):

result = MockResult()
result.body = u'A HapiException with unicode \u8131 \xe2\x80\xa2\t'
Copy link
Author

Choose a reason for hiding this comment

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

Now this test works with adding u as prefix here.

# Note the following line is missing the 'u' modifier on the string,
# this is intentional to simulate poorly formatted input that should
# still be handled without an exception
request['data'] = "A HapiException with unicode \\u8131 \xe2\x80\xa2"
Copy link
Author

Choose a reason for hiding this comment

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

And \\ here.

@ruichen-wang ruichen-wang requested a review from pjsg December 10, 2019 18:36
@ruichen-wang ruichen-wang requested a review from pjsg December 11, 2019 16:33
@ruichen-wang ruichen-wang merged commit d13bdba into master Dec 11, 2019
@ruichen-wang ruichen-wang deleted the cmw/RP-60972-make-hapipy-to-be-python3-compatible branch December 13, 2019 17:43
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.

6 participants