Replace use of __cmp__ in twisted/python/versions.py

[vperic]

cmp is not supported as of Python 3, in favor of rich comparison operators. Refactor twisted/python/versions.py (and the appropriate tests) to use rich comparison. As this is simple enough, just rename the current function and wrap it with rich comparison operators. This also allowed me to remove an internal class from the file. I have also updated the tests to cover one case not previously covered.

[vperic]

First version

[vperic]

Second version, fix silly typo :-)

[exarkun]

Thanks for working on this!

A few simple things, relating to  Twisted's coding standard:

1. All methods (and classes) in Twisted need to have a docstring.
2. All code requires at least 100% line and branch coverage. Ideally all code is developed test-driven, so this happens naturally. Even when modifying existing code, though, if there is missing test coverage, it needs to be added before the change can be made to trunk. Use  coverage.py to generate coverage reports. For example, I did python-coverage run --branch --source twisted.python.versions bin/trial twisted/python/test/test_versions.py and then python-coverage html. An html report is dumped into a directory named "htmlcov". It reports some of the new lines aren't covered.

Some points more specifically about the code changes in the patch:

3. The code seems a bit repetitive. It would be neat to avoid having to have these six methods, all nearly identical. Perhaps the operator module would help with a more succinct expression of the behavior.
   1. It might also be nice to factor this out into a helper that can be re-used throughout Twisted, too.
4. Keeping _inf would seem to simplify _cmp quite a bit.
5. testDontAllowBuggyComparisons is not named according to the test naming convention, which is "test_someThing". Since you're modifying it, you should also rename it.
6. Also, that test method has no docstring, so you should add one.
7. Also, before the test method was invoking functionality via a public interface - comparison by way of the cmp builtin. Now it is invoking a private implementation detail. It'd be nicer to stick to the public interface. Perhaps the function passed to assertRaises should be something like operator.eq.

Also, one random point about trac's handling of keywords - they are space separated, not comma separated. If you put something like "py3k, review" into the keywords field, then the ticket has the "py3k," keyword.

Thanks again!

[thijs]

(In [34948]) Apply versions-5785-2.patch: Replace the usage of file with open in twisted.python.versions for Python 3 compatibility.

Author: thijs Reviewer: antoine, vperic Fixes: #5785 Refs #5627

[thijs]

#4305 was marked as duplicate.

[vperic]

(In [34972]) Create branch versions-cmp-5627

[vperic]

(In [34974]) Create branch versions-cmp-5627-2

[vperic]

Ok, the new branch has new code. Rich comparison is implemented in Version and the _inf helper object is kept. All new code is covered and other review comments are addressed.

About code duplication, there really is no way around it. We can omit some methods and Python magickry will allow it to still work, but this is a cleaner solution. One alternative could be using the @total_ordering decorator, but the version provided in Python 2.7 is buggy, so we'd have to carry our own version... I don't consider it to be worth it, as there are only a few uses of cmp in Twisted. In any case, I don't think that decision should stop this pull request - if we do decide to use total_ordering, it will be easy to remove some of the methods and use the decorator instead.

This branch also adds a superior version of the test which caused #5785 to be reverted (named test_notImplementedComparisons here).

 Build results

[itamar]

functools.total_ordering, even the version that doesn't have the bug vperic mentions, cannot be used, since it doesn't support NotImplemented correctly. Probably the thing to do is generalize the code already written into a CmpMixin of some sort; it's already fairly generic. I think _inf has to actually DTRT regarding all comparison variants, so maybe that generic mixin should be done as part of this ticket.

[itamar]

OK, I've updated the branch to use a mixin to emulate __cmp__ (which can also be used for other parts of Twisted that use __cmp__), as well as a version of cmp() for use in Python 3.

[exarkun]

1. Test method docstrings shouldn't use the word "should".
2. cmp and its test could be implemented in a way such that the test exercises our version of the code on all versions of Python, which would make coverage easier to verify and temporary regressions less likely.
3. And really, unit tests should have about one assertion in them, so that all problems are visible from one test run's results. Some of the existing unit tests are particularly bad offenders on this front, but I'm only really complaining about the brand new unit tests for now.
4. There are some classic classes in Twisted that override __cmp__, so it would be useful to have a classic version of CmpMixin. We could introduce that when we get to porting those classes though, I suppose.
5. Any reason to rename the special helper from __cmp__ to _cmp?
6. Does this slow down a bunch of stuff now, by forcing several extra Python function calls per comparison? Should we try to gut CmpMixin when running on a version of Python that supports __cmp__ natively (PyPy JIT can probably deal with this, and maybe we don't care about CPython performance anyway)?

Thanks.

[itamarst]

(In [35319]) Branching to 'versions-cmp-5627-3'

[itamar]

1. Done (for new tests, anyway).
2. I disagree, at least in this particular case where we know we'll have buildslaves for both versions. Python 2 should test the version it uses, Python 3 tests the version it uses. This tells us that Python 2 and 3 have same semantics. If we only test custom version (which Python 2 doesn't use), we don't know that we're matching the original Python 2 semantics in the Python 3 workaround.
3. Done for new tests.
4. Switched to class decorator, which should solve that.
5. Class decorator fixes that.
6. Class decorator fixes that.

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/versions-cmp-5627-3

[exarkun]

Thanks. Here's a quick review:

1. Docstring for test_lessThan has a mistake in it
2. It doesn't look like comparable does nothing on Python 2.
3. Skipping the test on Python 2 doesn't seem to offer any benefits. The tests are lightning fast. But skipping them means we have no idea how comparable-decorated classes will behave on Python 2.

Otherwise looks good (I rushed a bit), merge when the above are addressed.

[itamar]

1. Fixed.
2. Fixed.
3. Having fixed Python 2 to not do anything, those tests fail because there is no __ne__ etc. on Python 2, e.g.

   [ERROR]
   Traceback (most recent call last):
     File "/usr/lib/python2.7/unittest/case.py", line 327, in run
       testMethod()
     File "/home/itamarst/Devel/Twisted/branches/versions-cmp-5627-3/twisted/test/test_compat.py", line 326, in test_notImplementedNotEquals
       self.assertEqual(Comparable(1).__ne__(object()), NotImplemented)
   exceptions.AttributeError: 'Comparable' object has no attribute '__ne__'
   
   twisted.test.test_compat.Python3ComparableTests.test_notImplementedNotEquals
   

   so I left them skipped. You can't test the behavior of a non-existent method... The behavior when we call the high level operator (< etc.) will be correct, but that's a Python implementation detail we probably don't have to worry about, and we don't test that on Python 3 either since that's a second-order effect.

[itamarst]

(In [35324]) Merge versions-cmp-5627-3: Python 3 support for cmp and cmp(), initially for twisted.python.versions.

Author: vperic, itamar Review: exarkun Fixes: #5627

Added a class decorator to twisted.python.compat that lets cmp work on Python 3, as a well as a simple implementation of cmp(). These are used by twisted.python.versions now.