Opened 3 years ago

Closed 2 years ago

#5627 enhancement closed fixed (fixed)

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

Reported by: vperic Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords: py3k
Cc: thijs Branch: branches/versions-cmp-5627-3
(diff, github, buildbot, log)
Author: itamar, vperic Launchpad Bug:

Description

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.

Attachments (2)

cmp_versions.patch (4.6 KB) - added by vperic 3 years ago.
First version
cmp_versions_v2.patch (4.6 KB) - added by vperic 3 years ago.
Second version, fix silly typo :-)

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by vperic

First version

Changed 3 years ago by vperic

Second version, fix silly typo :-)

comment:1 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:2 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to vperic
  • Status changed from assigned to new

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:

  1. 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.
  2. Keeping _inf would seem to simplify _cmp quite a bit.
  3. testDontAllowBuggyComparisons is not named according to the test naming convention, which is "test_someThing". Since you're modifying it, you should also rename it.
  4. Also, that test method has no docstring, so you should add one.
  5. 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!

comment:3 Changed 2 years ago by thijs

  • Cc thijs added
  • Milestone set to Python-3.x

comment:4 Changed 2 years ago by 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

comment:5 Changed 2 years ago by thijs

#4305 was marked as duplicate.

comment:6 Changed 2 years ago by vperic

  • Author set to vperic
  • Branch set to branches/versions-cmp-5627

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

comment:7 Changed 2 years ago by vperic

  • Branch changed from branches/versions-cmp-5627 to branches/versions-cmp-5627-2

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

comment:8 Changed 2 years ago by vperic

  • Keywords review added
  • Owner vperic deleted

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

comment:9 Changed 2 years ago by itamar

  • Milestone changed from Python-3.x to Python 3.3 Minimal

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by itamar

  • Author changed from vperic to vperic, 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.

comment:12 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:13 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  • Priority changed from low to normal
  • Status changed from assigned to new
  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.

comment:14 Changed 2 years ago by itamarst

  • Author changed from vperic, itamar to itamarst, itamar, vperic
  • Branch changed from branches/versions-cmp-5627-2 to branches/versions-cmp-5627-3

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

comment:15 Changed 2 years ago by itamar

  • Author changed from itamarst, itamar, vperic to itamar, vperic
  • Keywords review added
  • Owner itamar deleted
  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

comment:16 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to itamar

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.

comment:17 Changed 2 years ago by 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.

comment:18 Changed 2 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Note: See TracTickets for help on using tickets.