On 10/15/2013 04:19 AM, Arun Babu Neelicattu wrote:
Submitting patches for this.
On Thu, 2013-10-10 at 18:05 +1000, Arun Babu Neelicattu wrote:
> Alright, finally got back to this.
>
> This should now be complete as requested earlier. All tests are now
> passing, basic + functional (ro/rw) on python2 (2.7 tested) and python 3
> (3.3 tested).
>
> The smallest change set for Python3 support is:
>
https://github.com/abn/python-bugzilla/commit/6f0bd169fa4748536eefd6c0bad...
>
>
> I have also added helper scripts for development/testing and updated
> HACKING file:
>
https://github.com/abn/python-bugzilla/commit/f9914db082a26a5bb60541e1701...
>
> I have not made any pep8 changes to any existing code.
>
> Any review might be good :)
>
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