Db Field instances cannot be compared to other objects

[Simon Charette]

​`django.db.models.fields.Field.__cmp__` assumes that it's always compared to a another Field instance thus raising an AttributeError when the following code is executed:

from django.db.models.fields import Field

Field() == None

It should make sure that other is a Field instance before accessing the creation_counter property and if not return NotImplemented.

I stumbled on this issue when trying to use a PickledObjectField to store Field instance. When calling full_clean on the field it raised an AttributeError because it was trying to compare it EMPTY_VALUES: value in EMPTY_VALUES.

I agree that this is NOT a common use case of the Field class however I think that Field instances should be able to compare themselves to other objects without raising a confusing AttributeError.

I'm attaching a patch that sticks with simple comparison (although we should REALLY move to advanced comparison) with tests.

Here's a chat log I had on #python concerning simple comparison:

charettes 13:31:23
What should an object that implements __cmp__ method should return if it can't compare to other.
papna 13:31:49
charettes: 1) Don't write __cmp__ methods unless you're targetting mesozoic Python 2) NotImplemented
charettes 13:32:41
papna: But the doc doesn't say that... it says only to use it for advanced comparaison
charettes 13:32:58
papna: http://docs.python.org/reference/datamodel.html#object.__cmp__
papna 13:33:42
charettes: Why are you using __cmp__?
charettes 13:34:42
papna: actually I'm writing a patch for a project that used this method... should I suggest to switch to adavanced comparaison?
papna 13:36:43
charettes: IIRC __cmp__ still uses the paradigm that __eq__ and friends and __add__ and friends and plenty of other stuff does, but I might be wrong. It's not flexible, which is why it has been deprecated well over a decade.
charettes 13:37:27
papna: aight ty for your time

I can adapt the current patch to use advanced comparison instead if you prefer.

[Simon Charette]

Added an improved patch that rely on __lt__ instead of __cmp__ since this is really only used for sorting anyway. All tests pass on trunk.

[Claude Paroz]

Note to the committer: transform the test comment in a docstring and replace __cmp__ by __lt__ in the same comment. Apart from that, looks good and tests pass.

[Simon Charette]

I'm no committer but I updated the match according to your note :)

[Anssi Kääriäinen]

Maybe I am over-complicating this ... but I think we should define total ordering for the Field class. This could be done by adding total_ordering support in utils.functional (*), adding __eq__ for Field and then just decorating the Field class with @total_ordering. Defining total ordering is a ​PEP-8 recommendation.

(*) Total ordering is available in Python 2.7 from functools, for 2.6 it could be just added by copy-pasting the functools code to utils/functional.py, or by using this snippet: ​http://code.activestate.com/recipes/576685-total-ordering-class-decorator/.

It would be best to have the total_ordering support as a separate patch (if not separate ticket). The Field total_ordering support would then be an easy addition using charettes' patch.

[Simon Charette]

I moved this to a ​pull request. Do you think this should be set to DDN?

[Simon Charette]

Added total_ordering decorator to utils.functionnal

[Simon Charette]

Updated ​pull request to use total_ordering.

[Anssi Kääriäinen]

A couple minor notes:

• A mention of where the code of "our" implementation of total_ordering comes from (Python 2.7 I guess?)
• A very simple test case for total_ordering? (A class that defines __lt__ and __eq__, uses @total_ordering, and then test two instances with <, >, <=, >=.)

It would be nice to get another opinion if this is overcomplicating... But IMO not, the decorator might find other uses, and following PEP-8 is always a good choice.

[Simon Charette]

I don't want to misinterpret his comment but Alex said ​it looked fine with `@total_ordering` on github.

I updated the pull request with a comment explaining that the code is borrowed for python 2.7.3.

Are you sure a TestCase is needed? I can write it but I feel like it must be already tested if it's included in stdlib?

[Anssi Kääriäinen]

Fixed in commit 5cbfb48b92cb26a335f9c8c0f79d3390290103e2