With this patch set, the code will all be python3 compatible, and the container tests for Fedora 29 will run on an a system with python2 removed entirely. Fedora 28 and CentOS will still be using Python2.
Note that this is based on top of #312.
rebased onto 866f7b6dc12379fdb9d8794b98f8a5af9df87221
Can't we also revert a4abfb230e9fd2a90959486461d63dcae81c7d3c now, since it'll be able to use Python 3 where FreeIPA is only offers Python 3?
Technically, sure. But if the CLI gives us what we need, I don't think it gains us anything useful.
pycodestyle-3 (from python3-pycodestyle) is the actively maintained fork of pep8.
pycodestyle-3
python3-pycodestyle
pep8
Aside from the pep8/pycodestyle thing, this looks good to me, though I'm concerned about the lack of test results returned from Jenkins. Doesn't this have CI?
pycodestyle
pretty please pagure-ci rebuild
@abbra unfortunately, it looks like there's no ci.centos.org pipeline wired up anymore, so it needs to be reviewed and tested locally.
Sorted returns a list, so the iterkeys here sounds redundant, plain sorted(opts) would work the same (faster maybe).
This will probably break in Python 4. Why not just else?
I generally see a lot of six.iteritems() and friends for small dicts (such as kwargs). Maybe this is overkill? If you just use .items() you get the proper behavior on Python 3 and the slightly worse behavior on Python 2 without noticing any difference. But the code will be more readable.
six.iteritems()
.items()
The omnipresent conditional decoding/encoding also feels like the str/bytes distinction wasn't given much thoughts. I'd consider imagining the data flow and agree whether you want to work with sequence of bytes or text and only do encoding/decoding at boundary level. However if you are confident in your tests, trying to pretend they are interchangeable (sticking with the wrong Python 2 paradigm) will work as well. Sorry if this sounds pedantic, I was bitten by this before, I merely try to share the experience.
Excellent read: https://portingguide.readthedocs.io/en/latest/strings.html
How can we move this forward?
Is anybody out there?
Hi Miro, I will try to take a look soon, the patchset is huge ...
@simo Have you had a chance to look over this? I'm starting to work on shipping Ipsilon with this patchset integrated for openSUSE because we're in Python 3 only land there...
Sorry, not yet, working vacation happened in the middle.
what's the point of this raise ?
Except for the extraneous raise all look good to me. @puiterwijk if you can fix it I think we can merge
@puiterwijk do you want me to take over and fixe the raise as a follow commit ?
@simo Can you please just do it? I spoke to @puiterwijk at Flock about it and he wants to get this merged and a new release of Ipsilon with Python 3 support out the door.
I also will be contributing a spec for building it for SUSE distributions...
Ok, I am going to merge and then follow up with any changes as needed
Pull-Request has been merged by simo
With this patch set, the code will all be python3 compatible, and the container tests for Fedora 29 will run on an a system with python2 removed entirely.
Fedora 28 and CentOS will still be using Python2.