Provide a library for valueless named constants

[Jean-Paul Calderone]

#4671 proposed adding a lot of new symbolic constant features to a helper somewhere in Twisted. This is for the kind of symbolic constant that is just a name. These would be useful in Twisted for things like the state machines in twisted.web._newclient.

[Jean-Paul Calderone]

(In [33117]) Branching to 'named-constants-5382'

[Jean-Paul Calderone]

I grabbed the valueless constant parts of #4671 and dumped them here.​Build results, please review.

[Glyph]

Reviewing.

[Glyph]

Review comments delivered in person; you know what you doing.

[Jean-Paul Calderone]

The review feedback was basically that the API was not amenable to easy documentation, since it involved creating instances rather than classes. Also, for the same reason, there was no convenient place to put custom functionality related to the constants (eg isIdempotent for a set of HTTP request method constants).

So the branch now implements a class-based interface.

[Jean-Paul Calderone]

​Build results

[ivank]

I took a close look at constants.py and it made perfect sense after a short while. Nice work.

1. Needs a NEWS file.

2. The documentation documents using is to identity-compare, but this is not tested in NamedConstantTests.

3. Testing the hash of NamedConstant in test_hash makes me squeamish because hash() is sort of a Python implementation detail. If it's testing something useful, keep it, but explain in the test docstring why you're testing hash().

4. Have you considered adding a __slots__ to NamedConstant? People will be instantiating hundreds of these, and they don't need a __dict__ as far as I can tell.

5. In NamedConstantsTests.setUp, you "Create a fresh new L{NamedConstants} subclass for each unit test to use". But when I move the class out to module level, the tests still pass. Maybe you could add a test that actually requires it being in setUp? But anyway, this is not very important; just an oddity.

6. It's unclear whether I can subclass NamedConstant and NamedConstants. I bring this up because I see "custom functionality related to the constants" in a comment above. Could you document this? If you're sure the API can support this but don't want to document/test this yet, could you make a new ticket?

7. I don't see any unicode in constants.py but do see unicode in test_constants.py. I'm a little confused by this; could you document how unicode works here, and whether I can have a unicode NamedConstant with no ASCII equivalent?

8. Why do you sort by _index in _initialize and again in iterconstants? Probably for a good reason, but it's not obvious to me.

[Glyph]

Replying to ivank:

4. Have you considered adding a __slots__ to NamedConstant? People will be instantiating hundreds of these, and they don't need a __dict__ as far as I can tell.

This is an optimization. You should avoid proposing optimizations without proposing benchmarks as well; and it is good to always put this kind of proposal into a separate 'you might consider' section. exarkun, I'm sure, can figure that out from context, but when reviewing new users' contributions it often seems onerous to have to consider a few surprising new design ideas and bits of scope-creep with every review.

In particular, __slots__ has wacky and confusing semantics, and usually isn't worth it. For example, my understanding is that when running on PyPy, for example (where I would hope most performance-critical deployments would be moving), objects with __slots__ don't actually have any memory savings or speedup; in fact it's a bit slower because it has to mimic CPython's weird semantics around attribute access et. al. instead of following the usual code paths that PyPy has for such things.

[ivank]

I brought up __slots__ because there's essentially no hope of adding it later, given how it changes the semantics. And would you really need a benchmark to make a decision about this? (I would just multiply the size of an empty dict times how many constants you expect to have.) I was worried about the memory usage rather than the performance, and I'd expect users who want low memory use to stick with CPython for a while.

Anyway, if __slots__'s wacky semantics make this feature worse, never mind.

[Jean-Paul Calderone]

Thanks for the review, ivank.

1. Added in r33161
2. It's not tested in NamedConstantTests, but it is tested in NamedConstantsTests. I don't think there's any meaningful way to test it in NamedConstantTests, since the NamedConstant instances are created explicitly by the test methods there, and so any test would only break if the test were buggy or the Python runtime failed to implement is correctly. Maybe you just overlooked the test in NamedConstantsTests?
3. I added a comment about hash collisions in dict and set in r33162. Does that address your concerns?
4. I pretty much agree with glyph about __slots__, so I'll leave that off.
5. It's true that all the tests pass if METHOD is defined outside of setUp, but they stop covering 100% of the code paths in the implementation. I slightly changed the wording of the setUp docstring in r33163, but I don't know if I really got the idea across.
6. The documentation gives an example of subclassing NamedConstants :) I could add the isIdempotent example there, perhaps, to make it even more clear (okay, I did, r33166). As far as subclassing NamedConstant goes - you are not intended to subclass it (though quite possibly there will be subclasses in Twisted soon, I'm not sure). I could replace NamedConstant with a function that returns an instance of a private class; that would remove the question entirely, however it once again complicates the documentation, since I can no longer talk about NamedConstant instances. And now that you mention it, I also meant to make it impossible to instantiate NamedConstants subclasses... That done in r33164. In r33165 I added a warning against misuse of NamedConstant, maybe that's good enough to address the actual review point.
7. Oh well. It almost made sense with the old API. Identifiers are text, though, not bytes. I suppose I'll just forget about this... r33167.
8. Sorry. The sorting in _initialize isn't necessary here. I'll delete it, I guess (r33168). You'll see it again soon though. ;)

Thanks again for the review.

[Jean-Paul Calderone]

​Build results again

[ivank]

Thanks. You've addressed all of my points. A few things I missed last time:

1. Typo, I think: "This is initialized in via the" in the NamedConstants docstring.

2. Please add an @ivar for _initialized. What's initialized?

3. Could you rename _initialized to something longer and less likely to collide? I am paranoid about this after the TestCase debacle.

4. Could you rename _initialize to something longer and less likely to collide? I'm worried about this because I accidentally put a NamedConstant in an object subclass instead of a NamedConstants subclass, and given how NamedConstant automatically calls _initialize on its parent, this could cause some really weird stuff if someone else has their own _initialize. As a bonus, a longer name will make the traceback more helpful.

5. Please add a @return to iterconstants.

6. Please add a @param and @return to lookupByName.

I took another close look at constants.py, and tried it out in a real program, and everything else looks good to me. It might be worth having another reviewer take a look, though.

[ivank]

7. Please add a use of lookupByName to constants.xhtml, so people don't start using getattr.

[Jean-Paul Calderone]

Thanks! I think that's all taken care of (in r33171 and r33172).

[Jean-Paul Calderone]

(In [33173]) Fix two more tiny doc mistakes

refs #5382

[ivank]

Thanks. You've addressed all of my points.

1. I think those @ivars in NamedConstants should be @cvars.

2. The traceback in constants.xhtml should have ValueError: __doc__, not ValueError: __init__

[Jean-Paul Calderone]

(In [33175]) fix example output refs #5382

[Jean-Paul Calderone]

(In [33176]) Everything on this class is class scope, methods and attributes refs #5382

[Jean-Paul Calderone]

Thanks, fixed.

[Itamar Turner-Trauring]

Some problems with Lore:

1. constants.xhtml:mismatched close tag at line 67, column 4; expected </stdin> (from line 62, column 8). The way you made lore happy in general with > and < on <pre> elements is interesting; surprisingly xmllint is happy with it (and cleans it up), so I guess that's OK.
2. <code class="API" base="twisted.python.constants">names</code> tries to point at object twisted.python.constants.names, which doesn't exist.

Subclassing:

1. I was assuming subclassing an existing NamedConstants subclass would let me add new sets of constants. As it turns it out, the subclass is broken. Not supporting subclassing is fine, e.g. maybe the general case semantics are too difficult and so require extra checks that you are only adding constants. But there should be some documentation to that effect.

Code:

1. _EnumerantsInitializer.__get__ docstring is a bit confusing, since it isn't the one who actually replaces _enumerants, contrary to what it implies, but rather the method it calls. So maybe that should be explained in _initializeEnumerants.
2. For some reason I can do SET.FOO > SET.BAR and get the reverse of SET.BAR < SET.FOO... and I can't figure out why. Not really a problem though.
3. I am worried about how close NamedConstant and NamedConstants are. The code will break appropriately if you have a typo, e.g. subclass NamedConstant instead of NamedConstants, but the resulting AttributeError happens later and isn't as informative as it could be. Perhaps NamedConstant.__get__ should have a try/except for AttributeError, with additional useful debugging info ("My container must be a NamedConstants, not a " + cls). Or maybe the names should be more different?

Tests:

1. If I comment out cls._enumerantsInitialized = True in _initializeEnumerants(), all the tests still pass. There should probably be some test that catches this.

[Jean-Paul Calderone]

Thanks for the review.

I fixed the lore markup, added some more end-user documentation (don't subclass subclasses) and maintainer documentation (hopefully clarifying _EnumerantsInitializer). I also renamed NamedConstants to Names to try to differentiate the two types better. There are some new tests as well, covering that extra initialization functionality. I could disable ordering on NamedConstant instances by explicitly implementing the necessary methods to return NotImplemented. It's even a little tempting. The current behavior (inherited by default from object) just compares based on id; that makes it fairly unreliable, so hopefully no one will actually decide to depend on it. I left it as-is, anyway.

[Itamar Turner-Trauring]

Fixes all look good. One last thing I missed: you should add constants.xhtml to the howto index so people can find it :)

Once that's fixed, feel free to merge; I look forward to using this!

[Jean-Paul Calderone]

(In [33276]) Merge named-constants-5382

Author: exarkun Reviewer: glyph, ivank, itamar Fixes: #5382

Add twisted.python.constants, a library for defining named constants.