search

ucphhpc / migrid-sync

MiGrid workspace where master branch is kept strictly in sync with SF upstream svn repo. Any development or experiments should use a branch. You probably want to fork your own clone or work e.g. on the edge branch if you wish to contribute.
GNU General Public License v2.0
4 stars 4 forks source link

Cover make_hash on Python 3. #117

Open albu-diku opened 2 months ago

albu-diku commented 2 months ago

Depends on: https://github.com/ucphhpc/migrid-sync/pull/140

albu-diku commented 2 months ago

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 commented 1 month ago

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 commented 3 weeks ago

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