Opened 8 years ago
Closed 8 years ago
#5382 enhancement closed fixed (fixed)
Provide a library for valueless named constants
Reported by: | Jean-Paul Calderone | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | ivank | Branch: |
branches/named-constants-5382
branch-diff, diff-cov, branch-cov, buildbot |
Author: | exarkun |
Description
#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
.
Change History (24)
comment:1 Changed 8 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/named-constants-5382 |
comment:2 Changed 8 years ago by
I grabbed the valueless constant parts of #4671 and dumped them here.Build results, please review.
comment:3 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
comment:5 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Glyph to Jean-Paul Calderone |
Status: | assigned → new |
Review comments delivered in person; you know what you doing.
comment:6 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
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.
comment:8 follow-up: 9 Changed 8 years ago by
Cc: | ivank added |
---|---|
Keywords: | review removed |
Owner: | set to Jean-Paul Calderone |
I took a close look at constants.py
and it made perfect sense after a short while. Nice work.
- Needs a NEWS file.
- The documentation documents using
is
to identity-compare, but this is not tested inNamedConstantTests
.
- Testing the hash of
NamedConstant
intest_hash
makes me squeamish becausehash()
is sort of a Python implementation detail. If it's testing something useful, keep it, but explain in the test docstring why you're testinghash()
.
- Have you considered adding a
__slots__
toNamedConstant
? People will be instantiating hundreds of these, and they don't need a__dict__
as far as I can tell.
- In
NamedConstantsTests.setUp
, you "Create a fresh newL{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 insetUp
? But anyway, this is not very important; just an oddity.
- It's unclear whether I can subclass
NamedConstant
andNamedConstants
. 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?
- I don't see any unicode in
constants.py
but do see unicode intest_constants.py
. I'm a little confused by this; could you document how unicode works here, and whether I can have a unicodeNamedConstant
with no ASCII equivalent?
- Why do you sort by
_index
in_initialize
and again initerconstants
? Probably for a good reason, but it's not obvious to me.
comment:9 Changed 8 years ago by
Replying to ivank:
- Have you considered adding a
__slots__
toNamedConstant
? 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.
comment:10 Changed 8 years ago by
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.
comment:11 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Thanks for the review, ivank.
- Added in r33161
- It's not tested in
NamedConstantTests
, but it is tested inNamedConstantsTests
. I don't think there's any meaningful way to test it inNamedConstantTests
, since theNamedConstant
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 implementis
correctly. Maybe you just overlooked the test inNamedConstantsTests
? - I added a comment about hash collisions in
dict
andset
in r33162. Does that address your concerns? - I pretty much agree with glyph about
__slots__
, so I'll leave that off. - It's true that all the tests pass if
METHOD
is defined outside ofsetUp
, but they stop covering 100% of the code paths in the implementation. I slightly changed the wording of thesetUp
docstring in r33163, but I don't know if I really got the idea across. - The documentation gives an example of subclassing
NamedConstants
:) I could add theisIdempotent
example there, perhaps, to make it even more clear (okay, I did, r33166). As far as subclassingNamedConstant
goes - you are not intended to subclass it (though quite possibly there will be subclasses in Twisted soon, I'm not sure). I could replaceNamedConstant
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 instantiateNamedConstants
subclasses... That done in r33164. In r33165 I added a warning against misuse ofNamedConstant
, maybe that's good enough to address the actual review point. - 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.
- 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.
comment:13 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Thanks. You've addressed all of my points. A few things I missed last time:
- Typo, I think: "This is initialized in via the" in the
NamedConstants
docstring.
- Please add an
@ivar
for_initialized
. What's initialized?
- Could you rename
_initialized
to something longer and less likely to collide? I am paranoid about this after theTestCase
debacle.
- Could you rename
_initialize
to something longer and less likely to collide? I'm worried about this because I accidentally put aNamedConstant
in anobject
subclass instead of aNamedConstants
subclass, and given howNamedConstant
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.
- Please add a
@return
toiterconstants
.
- Please add a
@param
and@return
tolookupByName
.
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.
comment:14 Changed 8 years ago by
- Please add a use of
lookupByName
toconstants.xhtml
, so people don't start usinggetattr
.
comment:15 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
comment:17 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Thanks. You've addressed all of my points.
- I think those
@ivar
s inNamedConstants
should be@cvar
s.
- The traceback in
constants.xhtml
should haveValueError: __doc__
, notValueError: __init__
comment:19 Changed 8 years ago by
comment:20 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Thanks, fixed.
comment:21 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Some problems with Lore:
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.<code class="API" base="twisted.python.constants">names</code>
tries to point at objecttwisted.python.constants.names
, which doesn't exist.
Subclassing:
- 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:
_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
.- For some reason I can do
SET.FOO > SET.BAR
and get the reverse ofSET.BAR < SET.FOO
... and I can't figure out why. Not really a problem though. - I am worried about how close
NamedConstant
andNamedConstants
are. The code will break appropriately if you have a typo, e.g. subclassNamedConstant
instead ofNamedConstants
, but the resulting AttributeError happens later and isn't as informative as it could be. PerhapsNamedConstant.__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:
- If I comment out
cls._enumerantsInitialized = True
in_initializeEnumerants()
, all the tests still pass. There should probably be some test that catches this.
comment:22 Changed 8 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
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.
comment:23 Changed 8 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
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!
comment:24 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [33117]) Branching to 'named-constants-5382'