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

Encode object before hashing #60

Closed
wants to merge 1 commit into from
Closed

Conversation

juga0
Copy link
Contributor

@juga0 juga0 commented Apr 18, 2020

When parsing cached files the following exception is raised:
TypeError: Unicode-objects must be encoded before hashing

Encoding before hashing solve this issue.

This patch fails for python < 3.6 (https://travis-ci.org/github/juga0/stem/builds/676610639), so it needs to be modified to support those versions.

When parsing cached files the following exception is raised:
`TypeError: Unicode-objects must be encoded before hashing`

Encoding before hashing solve this issue.
@atagar
Copy link
Contributor

atagar commented Apr 18, 2020

Hi juga. Stem's master branch requires python 3.6 or above so there's no need to support lower versions.

Could you please provide a unit test that reproduces this issue so I can see it (and prevent future regressions)?

Thanks!

@arthtyagi
Copy link

Hey @atagar but wouldn't backwards compatibility be a good thing?

@juga0
Copy link
Contributor Author

juga0 commented Jun 11, 2020

Hi, sorry i haven't written the test yet.
If it is fast for any of you, feel free to fork this branch, add the test and then merge that.
Otherwise i'll try to find some time.

@atagar
Copy link
Contributor

atagar commented Jun 12, 2020

Hey @atagar but wouldn't backwards compatibility be a good thing?

Nope, indefinite backward compatibility hurts. We use semantic versioning, for more information see...

https://stem.torproject.org/change_log.html#versioning

@arthtyagi
Copy link

Hey @atagar but wouldn't backwards compatibility be a good thing?

Nope, indefinite backward compatibility hurts. We use semantic versioning, for more information see...

https://stem.torproject.org/change_log.html#versioning

Oh alright, I read more about semantic versioning and it makes more sense. Thanks for letting me know :)

vEpiphyte added a commit to vertexproject/stem that referenced this pull request Oct 1, 2020
@vEpiphyte
Copy link

@atagar Sorry to message you directly on this PR, but I've been exploring this due a desire to use the caching; but even with these changes, I'm unable to get the file caching to work at all with real-world tests of a patched version of the 1.8.0 STEM library, trying both the proposed PR here and similar changes from the master branches collector.py. There also appears to be no test coverage for this functionality either - is this test omission intentional?

@atagar
Copy link
Contributor

atagar commented Oct 1, 2020

Hi vEpiphyte. As mentioned earlier on this ticket juga's branch is awaiting a unit test that reproduces the problem they mentioned so no, the lack of coverage is not intentional.

If somebody has code to reproduce a problem I'd be delighted to take a patch, but otherwise I'll probably close this PR at some point.

@vEpiphyte
Copy link

Would you prefer a reproduction case/test against the 1.8.0 tag or master ?

@atagar
Copy link
Contributor

atagar commented Oct 1, 2020

Against the master branch, please.

@atagar
Copy link
Contributor

atagar commented Oct 29, 2020

Closing. Feel free to reopen if we have a repro for the issue being addressed.

@atagar atagar closed this Oct 29, 2020
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.

4 participants