Opened 5 years ago

Closed 4 years ago

#6523 task closed fixed (fixed)

Constants HOWTO doesn’t document that constants are ordered.

Reported by: Wilfredo Sánchez Vega Owned by: Wilfredo Sánchez Vega
Priority: normal Milestone:
Component: core Keywords:
Cc: Glyph, Wilfredo Sánchez Vega Branch: branches/ordered-constants-6523-2
branch-diff, diff-cov, branch-cov, buildbot
Author: wsanchez

Description (last modified by Wilfredo Sánchez Vega)

The ordered and comparable behavior of constants would be useful to document:

>>> from twisted.python.constants import NamedConstant, Names
>>> class Letters(Names):
...   a = NamedConstant()
...   b = NamedConstant()
...   c = NamedConstant()
... 
>>> Letters.a < Letters.b < Letters.c
True
>>> Letters.a > Letters.b 
False
>>> Letters.c < Letters.b < Letters.a
False
>>> Letters.c > Letters.b 
True
>>> sorted([Foos.b, Foos.a, Foos.c])
[<Foos=a>, <Foos=b>, <Foos=c>]
>>> 

Change History (50)

comment:1 Changed 5 years ago by Wilfredo Sánchez Vega

Description: modified (diff)

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

Hm. It also doesn't explicitly implement this, and there are no unit tests for it. So we should think about whether we really like this behavior (it seems fine I guess, but since names have no values apart from their names I think it's not a *totally* obvious feature). If we do, then it also needs to be unit tested. And presumably we want to consider whether this also makes sense for the other constant types, currently ValueConstant and FlagsConstant.

comment:3 Changed 4 years ago by Wilfredo Sánchez Vega

Hrm. I’m kind of a fan, but I could see an argument that ValueConstants should order by value instead of by definition order. Though that would be a change:

>>> from twisted.python.constants import ValueConstant, Values
>>> class Letters(Values):
...  c = ValueConstant("c")
...  a = ValueConstant("a")
...  f = ValueConstant("f")
...  d = ValueConstant("d")
...  b = ValueConstant("b")
...  e = ValueConstant("e")
... 
>>> sorted(Letters.iterconstants())
[<Letters=c>, <Letters=a>, <Letters=f>, <Letters=d>, <Letters=b>, <Letters=e>]
>>> 

Also flags:

>>> from twisted.python.constants import FlagConstant, Flags
>>> class Letters(Flags):
...   c = FlagConstant()
...   a = FlagConstant()
...   f = FlagConstant()
...   d = FlagConstant()
...   b = FlagConstant()
...   e = FlagConstant()
... 
>>> sorted(Letters.iterconstants())
[<Letters=c>, <Letters=a>, <Letters=f>, <Letters=d>, <Letters=b>, <Letters=e>]
>>> 

comment:4 Changed 4 years ago by Wilfredo Sánchez Vega

Well, to be clear, for FlagsConstant this isn’t inconsistent, since flags do have values, and the sort order for those would be the same as the definition order, since the definition order defines the values, but even if you are explicit, it goes with definition order:

>>> class Letters(Flags):
...   c = FlagConstant(3)
...   a = FlagConstant(1)
...   f = FlagConstant(6)
...   d = FlagConstant(4)
...   b = FlagConstant(2)
...   e = FlagConstant(5)
... 
>>> tuple(Letters.iterconstants())
(<Letters=c>, <Letters=a>, <Letters=f>, <Letters=d>, <Letters=b>, <Letters=e>)
>>> 

comment:5 Changed 4 years ago by Glyph

I'm inclined to say that we should sort the constants themselves by definition order in all cases.

  1. It's consistent with the way that iterconstants already orders its results.
  2. If you want to sort by value or by name, the value and the name are already accessible as public APIs, but there's no way to access the inherent ordering yet, and it seems a bit silly to make _index public as an attribute.
  3. If you want the comparison to match the cardinality of the values, just put the order of the definitions in the order of the values, since only comparison within a particular constants container makes sense anyway.
    1. Also to this point: if we're going to change ordering behavior we really ought to make the comparison of a constant from one container and a constant from another container raise an exception.
  4. Not all values are themselves orderable, but the constants representing them can still be.
  5. Finally, and least importantly, due to the ... unfortunate nature of Python 2's default sort order, I think this is already the way it works. This is a little silly, but not having to worry about compatibility is nice.

comment:6 Changed 4 years ago by Wilfredo Sánchez Vega

Owner: set to Wilfredo Sánchez Vega
Status: newassigned

OK, I’ll take a stab at implementing this with tests and explicit ordering instead of lucky Python implementation ordering.

comment:7 Changed 4 years ago by Wilfredo Sánchez Vega

Branch: ordered-constants-6523

comment:8 Changed 4 years ago by Wilfredo Sánchez Vega

Author: wsanchez
Branch: ordered-constants-6523branches/ordered-constants-6523

(In [38474]) Branch for #6523

comment:9 Changed 4 years ago by Wilfredo Sánchez Vega

r38475 adds ordering methods and tests.

I'm not sure about "we really ought to make the comparison of a constant from one container and a constant from another container raise an exception". Right now comparisons with different types will return a result that may or may not be useful, but that seems to be the norm in Python:

>>> "1" > 1
True
>>> "1" > object()
True
>>> 

comment:10 Changed 4 years ago by Glyph

It's true that Python 2 has this property, but it's widely considered a bug and Python 3 has fixed it:

>>> "1" > 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unorderable types: str() > int()
>>> "1" > object()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unorderable types: str() > object()

comment:11 Changed 4 years ago by Wilfredo Sánchez Vega

Keywords: review added

I totally agree that's better, was just doubting whether in Python 2 we should be consistent with other Python 2 types here.

I prefer pedantic errors, so I'm in favor of raising.

I'll make it raise TypeError with a message similar to the Python 3 error above:

exceptions.TypeError: unorderable types: NamedConstant() > ValueConstant()

Done in r38476.

comment:12 Changed 4 years ago by Glyph

Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

In review, so, un-assigning.

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

I prefer pedantic errors, so I'm in favor of raising.

It seems this might be better, but it is backwards incompatible.

comment:14 in reply to:  13 Changed 4 years ago by Wilfredo Sánchez Vega

Replying to exarkun:

I prefer pedantic errors, so I'm in favor of raising.

It seems this might be better, but it is backwards incompatible.

I sort of agree, which is an argument for returning NotImplemented and making this the Python interpreter's problem.

But at the same time, it's not backwards incompatible with what? Code that's already only working by luck? Is there any reasonable assumption to be made about the ordering of constants? Perhaps: "there is some order and I don't care what it is" so that sorted(list_of_constants) gives me the same order each time and doesn't raise.

Unrelated but also on the compatibility front, I noticed that iterconstants() sorts the constants, which I think is not such a great idea one we have this code in place, but I didn't change it because someone might depend on it, even though it's not documented. But iterators really shouldn't sort for you; you should do sorted(Foo.iterconstants()) instead, which is now possible, so that you don't pay the costs of sorting unless you need it.

comment:15 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Wilfredo Sánchez Vega
  1. If revert the code changes (but not the test changes), the only thing that fails is test_orderedFlagConstants and test_orderedMixedConstants
    1. This suggests that the current behavior happens to be the same, in most cases, which is good.
    2. The ordering of composite flags depends on the order of how the are defined, which suggest that having an order for compsoite flag constants isn't useful. Or at least not this order, perhaps they should be ordered lexicographically among the non-composite constants.
  2. Constants aren't actually guarenteed by this code to be in definition order, but instantiation order of _Constant, but I don't think this is a problem in practice.
  3. I agree with exarkun that starting to raise an error is probably incompatible. I think returning NotImplemented is a good solution. If you want, you could perhaps issue a deprecation warning before doing that.
  4. iterconstants sorts things in the same order as this implements. I think that is a reasonable thing, since otherwise we get a random order from the dict they happen to be stored in.
    • In which case, there should be a test of this, but it could reasonably be in a different ticket.
  5. This doesn't update the documentation as described in the ticket documentation.
  6. The new _Constant methods should have docstrings

Please resubmit for review after addressing 1.2, 3, 5, 6 and optionally 4.

comment:16 Changed 4 years ago by Wilfredo Sánchez Vega

1.1: cool

1.2: I had to read that sentence about composite flags to understand the issue you raise, but I think I get it and agree.

2: ok

3: I still have to ask, compatible with what? There are no existing tests cases that fail if you change this; so the current behavior is accidental.

4: I actually think it's a dumb thing that iterconstants does any sorting, especially given that prior to this ticket being fixed, the sort order is undefined. If you want sorting, you should ask for it. As it is, you pay the cost of sorting even if you don't care what the order is. But I agree that's another ticket.

5: Huh.

6: Aren't __lt__ and the like already documented interfaces?

comment:17 Changed 4 years ago by Tom Prince

3: I still have to ask, compatible with what?

Existing behavior. Twisted tends to take a very wide view of compatability, even of untested behavior. There isn't hard rules for what compatibility means, although it is partly documented here.

It seems to me that raising an exception where there wasn't one (and where python2 doesn't typically raise one) is liable to cause things to break in code that inadvertanly compared constants from differnt containers.

4: I actually think it's a dumb thing that iterconstants does any sorting, especially given that prior to this ticket being fixed, the sort order is undefined

Looking at the code, it actually looks like the order is defined to be exactly the one that is implemented in this ticket.

5: Huh.

"Constants HOWTO doesn’t document that constants are ordered."

Unless I am blind, it still doesn't.

6: Aren't __lt__ and the like already documented interfaces?

_orderable isn't. In any case, the other docstrings seem like a good place in the api docs to document the ordering behavior.

There are only a few places where we leave out docstrings (besides the docstrings missing due to an evolving coding-standard), to be able to inherit them from an interface/superclass, but only when there is nothing signifigant to add about the behavior (and this is mostly because our documentation tools give a better result in that situation). There is something useful to say about these methods *and* they don't inherit a docstring.

(Typically this is done with cmp (and twisted.python.compat.comparable), but that overides __eq__, which I'm not sure we want to do here)

comment:18 Changed 4 years ago by Jonathan Jacobs

Somewhat in contrast to what Tom said, it would be nice if this code worked (and I can't think of any reason why you wouldn't want it to):

>>> from twisted.python.constants import Flags, FlagConstant
>>> class Things(Flags):
...   FOO = FlagConstant(1)
...   BAR = FlagConstant(2)
...
>>> (Things.FOO | Things.BAR) == (Things.FOO | Things.BAR)
False

comment:19 Changed 4 years ago by Glyph

Cc: Glyph added

comment:20 Changed 4 years ago by marco

Actually constants are ordered by accident. This happened when I was testing[1]:

>>> max(LogLevel.iterconstants())
<LogLevel=info>
>>> map(id, LogLevel.iterconstants())
[140099930058576, 140099930080336, 140099930079888, 140099930080016]

I cannot say what happened between the definition of info and warn but I guess it is because of the GC.

[1] http://trac.calendarserver.org/browser/CalendarServer/trunk/twext/python/log.py

comment:21 Changed 4 years ago by Wilfredo Sánchez Vega

marco: yeah. This should be fixed by the [source:branches/ordered-constants-6523/twisted/python/constants.py#L45 added explicit comparators] on _Constant.

comment:22 Changed 4 years ago by Wilfredo Sánchez Vega

r40056 removes the raising of TypeError when comparing constants with another type and removes the test that checks this behavior. This should address Tom's item #3.

I still think that explicitly breaking code that it broken and appears to be working, and that being compatible with broken code is a weird priority to assert over ensuring correctness, but that needn't stop progress on the rest of this.

comment:23 Changed 4 years ago by Wilfredo Sánchez Vega

Toms right about #4, the sort order returned by iterconstants() is exactly how I'm sorting things now. That said, I still don't want to implement a test for this, because (a) it's not documented behavior and (b) which it seems a reasonable thing to do, it's really not a good idea. Now that it's actually possible for the called to sort the result; that's how it should be done. Either way, it's for another ticket to sort out.

comment:24 Changed 4 years ago by Wilfredo Sánchez Vega

On #5, by "Huh", what I mean was "Whoops, totally meant to add that".

Added in r40057. Other cleanup in r40058.

comment:25 Changed 4 years ago by Wilfredo Sánchez Vega

Status: newassigned

comment:26 Changed 4 years ago by Wilfredo Sánchez Vega

#40059 adds docstrings to the comparison functions, addressing #6.

comment:27 Changed 4 years ago by Wilfredo Sánchez Vega

Er… that's r40059

comment:28 Changed 4 years ago by Wilfredo Sánchez Vega

jonathanj: I think you raise a good point there; those do look like they should work. More "obvious" examples:

>>> from twisted.python.constants import Flags, FlagConstant
>>> class Things(Flags):
...   FOO = FlagConstant()
...   BAR = FlagConstant()
... 
>>> Things.FOO == Things.FOO | Things.FOO
False
>>> (Things.FOO | Things.BAR) == (Things.FOO | Things.BAR)
False
>>> 

The trick here is that you think these are the same because they have the same value. You should be comparing values, though, to get what you want here:

>>> Things.FOO.value == (Things.FOO | Things.FOO).value
True

The point being that two constants which are defined separately are not equal even if they have the same value:

>>> from twisted.python.constants import Values, ValueConstant
>>> class Stuff(Values):
...   FOO = ValueConstant(1)
...   BAR = ValueConstant(1)
... 
>>> Stuff.FOO == Stuff.BAR
False
>>> 

This isn't super explicit in the docs (at least not in the HOWTO), though it's more obvious in the case of Names.

I don't think that's related to ordering, so I suggest that be a separate ticket, so this one can more along.

comment:29 Changed 4 years ago by Wilfredo Sánchez Vega

Last item is Tom's comment 1.2 that the ordering of composite FlagConstants is not obviously useful. Here's an example:

>>> from twisted.python.constants import Flags, FlagConstant
>>> class Things(Flags):
...   FOO = FlagConstant()
...   BAR = FlagConstant()
... 
>>> FOOBAR1 = Things.FOO | Things.BAR
>>> FOOBAR2 = Things.FOO | Things.BAR
>>> id(FOOBAR1), id(FOOBAR2)
(4383414992, 4383415184)
>>> sorted((FOOBAR2, Things.FOO, Things.BAR, FOOBAR1))
[<Things=FOO>, <Things=BAR>, <Things={BAR,FOO}>, <Things={BAR,FOO}>]
>>> [id(c) for c in sorted((FOOBAR2, Things.FOO, Things.BAR, FOOBAR1))]
[4383415120, 4383415248, 4383414992, 4383415184]
>>> [c._index for c in sorted((FOOBAR2, Things.FOO, Things.BAR, FOOBAR1))]
[34, 35, 36, 37]
>>> 

Had to print the id's to see FOOBAR1 and FOOBAR2 as different. (I also printed the indexes.) We see that the order does follow instantiation order.

This thing is that while it's not an order of terribly great meaning, it is a well-defined order. Tom suggested I sort lexicographically, but that's kind of arbitrary as well. Plus: part of the sorting is by index, and the rest by something else?

Composite constants also don't necessarily have unique names, so you end up with possibly undefined ordering:

>>> FOOBAR1.name
'{BAR,FOO}'
>>> FOOBAR2.name
'{BAR,FOO}'
>>>

I'm inclined to leave this as definition order. It's consistent with the others, and the same code path will always produce the same order.

comment:30 Changed 4 years ago by Wilfredo Sánchez Vega

Cc: Wilfredo Sánchez Vega added
Keywords: review added
Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

Submitting for review.

comment:31 Changed 4 years ago by Jean-Paul Calderone

I filed #6878 for the ==/!= issue noted by jonathanj earlier.

comment:32 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Wilfredo Sánchez Vega

Thanks! In particular I am happy to see a combination of tests, documentation, and implementation all in one branch. :)

  1. doc/core/howto/constants.xhtml - feel free to re-wrap the text being modified to use semantic linefeeds as now recommended by the doc standard.
  2. twisted/python/constants.py - The rich comparison methods document that they will return NotImplemented for comparisons of constants that belong to different classes but they don't actually implement this behavior.
  3. twisted/python/test/test_constants.py
    1. When either of the assertions in test_orderedNameConstants fails it will do so with minimal useful information about why it has failed. Ideally the failure itself should suggest what is wrong. This test will just point out that *something* about > (or maybe <!) is broken. A simpler test involving just two constants and just one ordering operator would produce more easily understood failures (iow, turning this into several tests would probably be great).
    2. Ditto for the following two tests.
    3. The organization of these new assertions would perhaps be better if, eg, the NamedConstant tests went on to the existing NamedConstantTests class (although I see there is no ValueConstantTests and the FlagConstant tests are split up into several test cases, so... maybe there are arguments for a different organization. or maybe this is a good opportunity to introduce some symmetry).

These are relatively minor comments, I think: just about documentation and test factoring. Please address them and then merge. Thanks again!

comment:33 Changed 4 years ago by Wilfredo Sánchez Vega

Branch: branches/ordered-constants-6523branches/ordered-constants-6523-2

(In [41858]) Been too long, need to merge forward.

comment:34 Changed 4 years ago by Wilfredo Sánchez Vega

OK, I need to merge forward due to the Lore->Sphinx dance.

comment:35 Changed 4 years ago by Wilfredo Sánchez Vega

r41859 to clean up the formatting of the text, no real changes.

comment:36 Changed 4 years ago by Wilfredo Sánchez Vega

r41866 merges this work forward, so that gets us to the state prior to exarkun's review above.

comment:37 Changed 4 years ago by Wilfredo Sánchez Vega

exarkun comment #1 regarding semantic newlines was addressed by r41859 and preserved in r41866.

Regarding comments #3.1 and #3.2, I broke the tests up and reduced the number of constants. I did use three instead of two constants for flags and values, because I want to make sure that the ordering of the values attached to the constants aren't determining the sort order and added a comment to that effect in the tests. That's r41867.

comment:38 Changed 4 years ago by Wilfredo Sánchez Vega

I'd like to pass on comment #3.3, as I think it's sensible for the ordering tests to be together, as they are very similar and a fix to one probably benefits from being able to quickly see that there are other very similar tests in place.

comment:39 Changed 4 years ago by Wilfredo Sánchez Vega

And now I'm noticing that we need tests for >= and <=. Added.

comment:40 Changed 4 years ago by Wilfredo Sánchez Vega

(r41868 for <= and >=)

comment:41 Changed 4 years ago by Wilfredo Sánchez Vega

Status: newassigned

comment:42 Changed 4 years ago by Wilfredo Sánchez Vega

That leaves comment #2, to which I'll add that tests were lacking and that the docstring was also not really correct.

I changed this to say we'll return NotImplemented if C{other} is not a constant belonging to the same container, since even if they are of the same type, the "definition ordering" we are preserving here is of no value if they are defined in separate containers. Added tests to verify this.

That's all in r41870.

comment:43 Changed 4 years ago by Wilfredo Sánchez Vega

Forcing Buildbot on the branch…

Failed in documentation/api-documentation for reasons looking very much unrelated (no new errors in twisted.python).

Failed twistedchecker, fixed in r41872, now green.

py-select-gc is still yellow…

comment:44 Changed 4 years ago by Wilfredo Sánchez Vega

Keywords: review added

Back to review despite the OK to merge, since the changes after the last review, while straightforward, were not trivial, so another quick look might be in order.

comment:45 Changed 4 years ago by Wilfredo Sánchez Vega

Owner: Wilfredo Sánchez Vega deleted
Status: assignednew

comment:46 Changed 4 years ago by hawkowl

Owner: set to hawkowl

comment:47 Changed 4 years ago by hawkowl

Keywords: review removed

Hi, thanks for this!

A few comments:

  • twisted/python/constants.py, line 104 - since it's ge, shouldn't the C{>} be C{>=} or so?
  • twisted/python/test/test_constants.py - line 1109 - stray o?
  • line 1110 and 1124 - lets instead of let's
  • line 1111 and 1125 - "happen to get" rather than "happen the get"?
  • line 1124 and 1110 - "definition order is different from" rather than "definition order different from"

Other than that, looks good! Please fix these minor issues and merge.

-hawkowl

comment:48 Changed 4 years ago by hawkowl

Owner: changed from hawkowl to Wilfredo Sánchez Vega

comment:49 Changed 4 years ago by Wilfredo Sánchez Vega

Nice catches, hawk owl, thanks.

r41921

comment:50 Changed 4 years ago by Wilfredo Sánchez Vega

Resolution: fixed
Status: newclosed

(In [41925]) Merge ordered-constants-6523-2: Constants are now ordered to match definition order.

Author: wsanchez Reviewer: exarkun, hawkowl Fixes: #6523

The order in which constants are defined is now also the comparison order for constants. This is now explicitly implemented with tests and the documentation has been updated.

Note: See TracTickets for help on using tickets.