klipper icon indicating copy to clipboard operation
klipper copied to clipboard

scripts: Python3 migration

Open junland opened this issue 3 years ago • 4 comments

Here's my second attempt. I excluded whconsole.py and buildcommands.py as those two scripts would take a little more time / work to get migrated over. Also it was suggested in the last PR that setting the she bang to just python (Which most likely be symlinked to python3) so I did just that and left the scripts alone that explicitly required python2 for compatibility sake.

Signed-off-by: John Unland [email protected]

junland avatar Nov 29 '22 16:11 junland

I stumbled across another python 2/3 discrepancy in parsedump.py when expanding arc command support. It's included in that pull request but you can cherrypick/patch from klippy/parsedump.py directly.

ajmirsky avatar Nov 30 '22 14:11 ajmirsky

I stumbled across another python 2/3 discrepancy in parsedump.py when expanding arc command support. It's included in that pull request but you can cherrypick/patch from klippy/parsedump.py directly.

Thanks, I believe it was mentioned in my previous PR that whconsole.py would have this problem with strings / byte strings conversion. Though I'll hold off to send in that PR as I'm still rusty on my python.

junland avatar Nov 30 '22 16:11 junland

Thanks. I agree we should update these scripts so that they can run with either python2 or python3.

It's important that white space changes aren't mixed with functional changes - as mentioned at https://www.klipper3d.org/CONTRIBUTING.html .

A big commit containing all these changes is difficult to review and makes it hard to use tools like git bisect and git revert. I'd prefer to see one commit per tool, along with a declaration that the tool was tested on both python2 and python3. I'd rather the software clearly state if it is python2 or python3 then for it to have subtle bugs.

Cheers, -Kevin

KevinOConnor avatar Dec 02 '22 16:12 KevinOConnor

Thanks. I agree we should update these scripts so that they can run with either python2 or python3.

python2 was EOL'd / Sunset Jan 1st. 2020. While I understand that these scripts should run both language versions, there should be a long term goal of fully moving / requiring python3. One less thing to test against is always good. :)

It's important that white space changes aren't mixed with functional changes - as mentioned at https://www.klipper3d.org/CONTRIBUTING.html .

A big commit containing all these changes is difficult to review and makes it hard to use tools like git bisect and git revert. I'd prefer to see one commit per tool, along with a declaration that the tool was tested on both python2 and python3. I'd rather the software clearly state if it is python2 or python3 then for it to have subtle bugs.

Cheers, -Kevin

Gotcha, I'll reset the commits and see if I can make individual ones. I just put it as one big commit to reduce the commit log (it's also what we do at my work so its just a natural thing for me to do) Might be good to mention something about that in the contributing doc about number of commits in a PR.

junland avatar Dec 03 '22 20:12 junland

You can batch together changes together if you want, but one big commit seems a bit unwieldy. (It doesn't need to be one commit per tool.) The big thing is confidence that the tools will actually run correctly on python3.

Cheers, -Kevin

KevinOConnor avatar Dec 07 '22 18:12 KevinOConnor

Moving to this PR as I messed up my git history.

junland avatar Dec 11 '22 00:12 junland