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 some python3'isms #901

Merged
merged 1 commit into from Sep 23, 2017
Merged

fix some python3'isms #901

merged 1 commit into from Sep 23, 2017

Conversation

@jmgurney
Copy link
Contributor

@jmgurney jmgurney commented Sep 17, 2017

In attempting to get CryptoSign working, I ran into a few python3'isms that make the cryptosign file not compatible w/ python2.

I have not run this through tests on python3 as I'm not at all familiar w/ 3. From what I read, there may need to be a .encode('latin-1') or something similar on the _makepad call.

@jmgurney
Copy link
Contributor Author

@jmgurney jmgurney commented Sep 17, 2017

Looks like currently failing checks are issues w/ the CI environment and not this PR.

@om26er
Copy link
Member

@om26er om26er commented Sep 17, 2017

Looks like currently failing checks are issues w/ the CI environment and not this PR.

Yeah, I created an issue for that #900


def get_string(self):
return self.get_bytes(self.get_uint32())


def _makepad(l):
return ''.join(chr(x) for x in range(1, l + 1))

This comment has been minimized.

@meejah

meejah Sep 18, 2017
Member

I would choose a different name for this arg (size maybe?) as it's not that easy to distinguish 1 and l

@meejah
Copy link
Member

@meejah meejah commented Sep 18, 2017

Thanks for the PR with tests :) 👍
Looks good to me (besides maybe the one arg name). If you prefer, you can squash and force-push the branch

These may break python3, but at least we have a test now..

change arg to be better per meejah
@jmgurney jmgurney force-pushed the jmgurney:master branch from 474dd79 to 2ee571b Sep 18, 2017
@jmgurney
Copy link
Contributor Author

@jmgurney jmgurney commented Sep 18, 2017

Ok, I've done both... squashed them down.

After this gets merged, I have some other work that I'll make a PR for.

@oberstet oberstet merged commit b1b1148 into crossbario:master Sep 23, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@oberstet
Copy link
Member

@oberstet oberstet commented Sep 23, 2017

awesome, thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.