Hi Cole,
I misunderstood, just assumed you wanted a single py2 to py3 commit. I can do the split
over the weekend hopefully.
.gitignore changes: ack
setup.py changes: i think setuptools should handle it, will test further and see what I
can do to make it less 'hacky'.
tests/rw_functional.py refactoring: this was since the tests (original and new) raised
Fault as the test user did not have privileges to change the priority/severity and this
was uncaught. Just figured it was cleaner/extensible with the refactor. Do you want me to
leave it as is? Or just add a generic try/except with a generic failure message around
that block?
-arun
----- Original Message -----
From: "Cole Robinson" <crobinso(a)redhat.com>
To: "python-bugzilla user/developer list"
<python-bugzilla(a)lists.fedorahosted.org>
Cc: "Arun Neelicattu" <abn(a)redhat.com>
Sent: Wednesday, October 16, 2013 11:12:47 PM
Subject: Re: [python-bugzilla] Python 3 support
Thanks for following up.
In the future, please send patches using git send-email, makes reviewing
easier.
That said, these patches are difficult to review, they are still conflating
fairly trivial sed-like changes with extremely important functional changes
(pycurl->requests). I understand that doing this work in one lump is probably
easiest for you, but it makes review and maintenance harder. Maybe what I
said
earlier mislead you here since I asked for to 'boil the py3 patch down to the
smallest amount necessary', but I didn't mean it had to be a single commit :)
I'm realize that unwinding a large commit is frustrating and unfun, but
ideally this series would be the following distinct patches:
- print "foo" -> print("foo")
- sys.exc_info() change
- fix imports to work on python2/3
- remaining python3 issues
- convert to python-requests
- test suite setup for vs py3 (maybe this is earlier in the series)
- build system changes
It might make sense to re-arrange that or make it even more fine grained,
that's just off the top of my head.
So, for example, if that print() patch were on the list right now and
demonstratably didn't cause any test suite/pylint regression, I'd commit and
push it since it's trivial. But as is, I can't push this patch until I can
get
some time to verify there aren't any ssl or cookie regressions with the
python-requests support, in addition to the review overhead.
Most of the conversion looks fine, but a couple bits from the patches jumped
out at me:
.gitignore changes: stuff like .so and nosetests.xml don't apply to us, so I
don't want to track them. Even stuff like *.swp shouldn't be in our current
.gitconfig, people should configure a global .gitconfig and ignore bits
generated by their tools of choice. Expecting every project to track these
bits does not scale. So please limit the gitignore change to only bits that
are generated by any build system changes.
setup.py changes: conditionalizing get_requirements on the prescence of pip
is
a hack: sounds like it will break local RPM build testing, and we can't be
sure that pip won't be pulled in by some python-bugzilla depedency for fedora
builds. Any way to wire this in with --single-version-externally-managed ?
Maybe setuptools does it automatically.
tests/rw_functional.py refactors the priority/severity test for reasons I
can't discern.
Thanks,
Cole