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

Cover make_hash on Python 3. #117

Open
wants to merge 2 commits into
base: edge
Choose a base branch
from
Open

Cover make_hash on Python 3. #117

wants to merge 2 commits into from

Conversation

albu-diku
Copy link

@albu-diku albu-diku commented Sep 12, 2024

Depends on: #140

Copy link
Contributor

@jonasbardino jonasbardino left a comment

Choose a reason for hiding this comment

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

While this looks alright and I'd really like to get that unit test added I think we should briefly discuss if this is indeed the way we want to address code that has already been ported to py3 with various force_X wrappers in the experimental branch. E.g. if it should go through mig.shared.compat or similar. I mean, to limit the amount of changing back and forth in both branches.

@jonasbardino jonasbardino self-assigned this Sep 15, 2024
@jonasbardino jonasbardino added the unit test Code and infrastructure related to unit testing label Sep 15, 2024
@albu-diku
Copy link
Author

While this looks alright and I'd really like to get that unit test added I think we should briefly discuss if this is indeed the way we want to address code that has already been ported to py3 with various force_X wrappers in the experimental branch. E.g. if it should go through mig.shared.compat or similar. I mean, to limit the amount of changing back and forth in both branches.

Hm. So, I've hit multiple cases recently where use of the force functions (and thus the partial port) does not produce the same result. My stance thus has become I'll take working covered code over code that happens to be written with force functions. Once there is a test that can be run once is free to change them however one wants, inclucing to make use of the force functions, but the catc-h-22 of having no coverage has to be solved first. Ideally there would be no use of force in the Py2 tree, but unfortunately we're not there - and some of the "partial porting" that has been done doesn't work.

@jonasbardino
Copy link
Contributor

Sorry about the slow response. My point in the comment above was that AFAICT you've replaced the existing force_utf8 call with a built-in codecs.encode call directly. That is, rather than e.g. doing so through a new compat helper function or similar, which would indeed allow us to easily trace, make and test implementation changes.
So not only does the PR in effect introduce new code in edge but also leaves no obvious path ahead for what to do in experimental.

You're right about catch-22's lurking and I don't have a good solution at hand, but I think we need to decide if/what we want to backport to edge and how we do it with the least hassle and risk. You suggested shared.compat before and I think that sounds like a better way ahead if we intend to get edge fully functional with py3 - without effectively repeating the porting efforts from experimental.

Hmmm, I'm not sure I understand which "partial porting" that 'doesn't work' you refer to, but perhaps that's a matter of definition/expectations ... To my understanding we're fine as long as we use py2 with edge and py3 with experimental. Mixing python3 with edge is another matter, though.

@albu-diku albu-diku force-pushed the fix/make-hash-py3 branch 2 times, most recently from deb99dd to 787f546 Compare October 7, 2024 09:12
@albu-diku albu-diku force-pushed the fix/make-hash-py3 branch 3 times, most recently from 1905954 to 73fe08f Compare November 2, 2024 10:34
@albu-diku albu-diku changed the title Repair make_hash on Python 3. Cover make_hash on Python 3. Nov 2, 2024
@albu-diku
Copy link
Author

With the correct force_utf8 function for each version available this becomes a test addition only.

Import the experimental branch version of force_utf8 wholesale adding a
-py(2|3) suffix and expose the correct implementation dependent on PY2.

Include forcing InputException messages to a native string as is done in
experimental (also taken directly from that branch) which ensures the
exception message, which may be unicode, becomes a string everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unit test Code and infrastructure related to unit testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants