Opened 6 years ago

Closed 5 years ago

#5627 enhancement closed fixed (fixed)

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

Reported by: Vladimir Perić Owned by: Itamar Turner-Trauring
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords: py3k
Cc: Thijs Triemstra Branch: branches/versions-cmp-5627-3
branch-diff, diff-cov, branch-cov, buildbot
Author: itamar, vperic

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 Vladimir Perić 6 years ago.
First version
cmp_versions_v2.patch (4.6 KB) - added by Vladimir Perić 6 years ago.
Second version, fix silly typo :-)

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by Vladimir Perić

Attachment: cmp_versions.patch added

First version

Changed 6 years ago by Vladimir Perić

Attachment: cmp_versions_v2.patch added

Second version, fix silly typo :-)

comment:1 Changed 6 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:2 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Vladimir Perić
Status: assignednew

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 5 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Milestone: Python-3.x

comment:4 Changed 5 years ago by Thijs Triemstra

(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 5 years ago by Thijs Triemstra

#4305 was marked as duplicate.

comment:6 Changed 5 years ago by Vladimir Perić

Author: vperic
Branch: branches/versions-cmp-5627

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

comment:7 Changed 5 years ago by Vladimir Perić

Branch: branches/versions-cmp-5627branches/versions-cmp-5627-2

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

comment:8 Changed 5 years ago by Vladimir Perić

Keywords: review added
Owner: Vladimir Perić 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 5 years ago by Itamar Turner-Trauring

Milestone: Python-3.xPython 3.3 Minimal

comment:10 Changed 5 years ago by Itamar Turner-Trauring

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 5 years ago by Itamar Turner-Trauring

Author: vpericvperic, 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 5 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:13 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring
Priority: lownormal
Status: assignednew
  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 5 years ago by itamarst

Author: vperic, itamaritamarst, itamar, vperic
Branch: branches/versions-cmp-5627-2branches/versions-cmp-5627-3

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

comment:15 Changed 5 years ago by Itamar Turner-Trauring

Author: itamarst, itamar, vpericitamar, vperic
Keywords: review added
Owner: Itamar Turner-Trauring 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 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Itamar Turner-Trauring

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 5 years ago by Itamar Turner-Trauring

  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 5 years ago by itamarst

Resolution: fixed
Status: newclosed

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