[docs] Code, test, and doc review for PEP-0435 Enum (issue 17947)

ethan at stoneleaf.us ethan at stoneleaf.us
Sun May 12 14:47:01 CEST 2013


Reviewers: eli.bendersky, berkerpeksag, zach.ware, eric.araujo, ezio.melotti, alex,

Message:
Okay, just discovered the publish and mail portion.  This explains why
nobody was responding to my replies.  ;)


http://bugs.python.org/review/17947/diff/8105/Lib/enum.py
File Lib/enum.py (right):

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode2
Lib/enum.py:2: =========
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Yeah, remove this :-)

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode17
Lib/enum.py:17: 
On 2013/05/10 16:46:19, berkerpeksag wrote:
> Nit: Two blank lines. (see PEP 8 E302)

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode18
Lib/enum.py:18: class StealthProperty():
On 2013/05/10 16:46:19, berkerpeksag wrote:
> Is there a reason not to write `class StealthProperty:` (without the
brackets)
> here?

I just always use them.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode23
Lib/enum.py:23: # Used to provide access to the `name` and `value`
properties of enum
On 2013/05/10 16:02:43, eli.bendersky wrote:
> This class is quite generic though, and doesn't mention name or value
> properties. Document it in a generic way, and then inside the enum
class
> document how StealthProperty is actually being used.
> 
> Also, _StealthProperty as this is an internal class.

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode84
Lib/enum.py:84: if key[:2] == key[-2:] == '__' or hasattr(something,
'__get__'):
On 2013/05/10 16:02:43, eli.bendersky wrote:
> parenthesize the parts of the or

like:

if (key[:2] == key[-2:] == '__') or (hasattr(something, '__get__')):

?

Why?

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode85
Lib/enum.py:85: if key in self._enum_names:
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Wait what? This also contradicts the docstring. And why should we do
that? 
> 
> I see no reason to provide more safeguards in enum than regular Python
classes
> provide

Thought I had fixed the docstring.  Fixed now.

More safeguards for internal consistency: if we don't remove the name
from _enum_names then the descriptor would become an enum member, and
this is explicitly not allowed.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode92
Lib/enum.py:92: dict.__setitem__(self, key, something)
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Perhaps using super() here would be cleaner and more future-proof

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode94
Lib/enum.py:94: Enum = None  # dummy value until replaced
On 2013/05/10 16:02:43, eli.bendersky wrote:
> beefier comment that explains why this dummy is needed. Also, would
this need a
> docstring? I'm thinking of 
> 
> >> import enum
> >> help(enum.Enum)

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode97
Lib/enum.py:97: class EnumMeta(type):
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Should this be _EnumMeta too?

Whatever this name is will show up in `type(Color)`, and I think '<class
'enum._EnumMeta'> looks tacky.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode98
Lib/enum.py:98: """Metaclass for Enum
On 2013/05/10 16:02:43, eli.bendersky wrote:
> There's no point in repeating the docs here - *how* an enum behaves
belongs
> there. I'd suggest a detailed imlementation explanation instead here
in the
> code. I.e. how a new enum gets created, which metaclass methods get
invoked and
> what they do.
> 
> I'm not saying to put a metaclass tutorial here, but a bit of details
to help
> follow the flow of what happens when:
> 
> class Color(Enum):
>   red = 1
>   blue = 2
> 
> is defined

Hmmm.  This will take some thinking -- I'll come back to it.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode116
Lib/enum.py:116: # an Enum class is final once enumeration items have
been defined;
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Explain in more detail what a "data type" is

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode118
Lib/enum.py:118: # __new__ unless a new __new__ is defined (or the
resulting class
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Hmm, this __new__ protocol has to be documented on a higher level
first (in the
> docs?)

It's the same __new__ you find in the normal Python docs.  The
mind-bending part is that it's only used for construction of the enum
members -- after that it is replaced with Enum's __new__ (but saved in
case of subclassing)

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode127
Lib/enum.py:127: and issubclass(base, Enum)
On 2013/05/10 16:02:43, eli.bendersky wrote:
> indentation

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode130
Lib/enum.py:130: if not issubclass(base, Enum):
On 2013/05/10 16:02:43, eli.bendersky wrote:
> make it more explicit that base here is the last base!

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode133
Lib/enum.py:133: # get correct mixin_type (either mixin type of Enum
subclass,
On 2013/05/10 16:02:43, eli.bendersky wrote:
> theres no mixin_type in the code. also close the paren )

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode159
Lib/enum.py:159: if __new__ is None:
On 2013/05/10 16:02:43, eli.bendersky wrote:
> if would be great to make this method shorter. helper methods with
well-defined
> functionality and well-chosen names go a long way towards code
readability

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode240
Lib/enum.py:240: def __call__(cls, value, names=None, *,
module_name=None, type=None):
On 2013/05/10 16:02:43, eli.bendersky wrote:
> Hmm, PEP 435 says "module" not "module_name". While the latter is more
explicit,
> the former is shorter and it's a rather specialized case. I'll be on
the fence
> about this.

Changed back to `module`.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode244
Lib/enum.py:244: class_name = value  # better name for a name than value
;)
On 2013/05/10 16:02:43, eli.bendersky wrote:
> I'd suggest a new method here:
> 
> _make_enum_from_names(name, member_names, ....)

Not much happens before this point, and everything after this is about
making the enum class -- I don't see that breaking off 90% of this
method into another buys us much.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode280
Lib/enum.py:280: return cls._enum_map.copy()
On 2013/05/10 16:02:43, eli.bendersky wrote:
> why copy?

If we return the actual map, and it gets modified, our Enum class just
got destroyed (or at least corrupted).

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode318
Lib/enum.py:318: enum_member = cls._enum_map[enum_name]
On 2013/05/10 16:02:43, eli.bendersky wrote:
> why don't you just search enum_map directly?
> 

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode344
Lib/enum.py:344: @StealthProperty
On 2013/05/10 16:02:43, eli.bendersky wrote:
> OK, here a brief explanation of how _StealthProperty is actually used
*in Enum*
> and why

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode356
Lib/enum.py:356: if __name__ == '__main__':
On 2013/05/10 16:02:43, eli.bendersky wrote:
> remove this

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/test/test_enum.py
File Lib/test/test_enum.py (right):

http://bugs.python.org/review/17947/diff/8105/Lib/test/test_enum.py#newcode1
Lib/test/test_enum.py:1: #!/usr/bin/python3.3
On 2013/05/10 18:30:30, eric.araujo wrote:
> I’d just delete the shebang actually, it won’t be useful in the
stdlib.

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/test/test_enum.py#newcode8
Lib/test/test_enum.py:8: print('Using Python', sys.version)
On 2013/05/10 17:30:54, zach.ware wrote:
> Extra output isn't very well appreciated in the test suite output.

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/test/test_enum.py#newcode54
Lib/test/test_enum.py:54: print(exc)
On 2013/05/10 17:30:54, zach.ware wrote:
> Since all of these above are simply creating enums without any kind of
special
> trickery going on, they really really shouldn't raise any exceptions. 
If they
> do, we should just let unittest handle them and flag the test as a
failure. 
> Taking out the try/excepts will also make Question's line short enough
:)

They shouldn't fail, but they have. ;)  They have to be in the module
name space for pickle tests, and if they raise an exception there no
other test will run.

Removed all the print statements.

http://bugs.python.org/review/17947/diff/8105/Lib/test/test_enum.py#newcode103
Lib/test/test_enum.py:103: '<Season.{0}: {1}>'.format(season, i))
On 2013/05/10 17:30:54, zach.ware wrote:
> Whitespace between methods

Done.

http://bugs.python.org/review/17947/diff/8105/Lib/test/test_enum.py#newcode634
Lib/test/test_enum.py:634: '''stub for testing one at a time'''
On 2013/05/10 17:30:54, zach.ware wrote:
> This should be removed.

Done.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py
File Lib/enum.py (right):

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode2
Lib/enum.py:2: Provides the Enum class, which can be subclassed to
create new, static, enumerations.
On 2013/05/10 20:18:59, ezio.melotti wrote:
> Line too long.

Done.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode8
Lib/enum.py:8: __all__ = ('Enum', 'IntEnum')
On 2013/05/10 18:30:30, eric.araujo wrote:
> Some tools (pydoc?) have issues if __all__ is not a list.

Done.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode86
Lib/enum.py:86: if key in self._enum_names and self[key] != something:
On 2013/05/10 20:18:59, ezio.melotti wrote:
> Does this mean than defining the same element twice with the same
value is
> allowed?
> Is there any reason to allow this?

Probably not, took it back out.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode87
Lib/enum.py:87: raise TypeError('Attempted to reuse key: %s' % key)
On 2013/05/10 20:18:59, ezio.melotti wrote:
> Using %r is probably better.

Done.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode207
Lib/enum.py:207: + list(self.__members__))
On 2013/05/10 20:18:59, ezio.melotti wrote:
> The operator should go at the end of the previous line.

Done.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode244
Lib/enum.py:244: and base._enum_names):
On 2013/05/10 20:18:59, ezio.melotti wrote:
> The operator should go at the end of the previous line.

Done.

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode270
Lib/enum.py:270: return obj_type, first_enum
On 2013/05/10 20:18:59, ezio.melotti wrote:
> You could simplify this a bit and save some indentation by doing:
> if not bases:
>     return object, Enum
> for base in bases:
>     ...
> 
> The same could also be done at line 252.

Good call.  Done.  (Couldn't do it at 252, though.)

http://bugs.python.org/review/17947/diff/8108/Lib/enum.py#newcode284
Lib/enum.py:284: for possible in (obj_type, first_enum):
On 2013/05/10 20:18:59, ezio.melotti wrote:
> You could use sets here.

I'm not sure how -- example?

http://bugs.python.org/review/17947/diff/8110/Lib/enum.py
File Lib/enum.py (right):

http://bugs.python.org/review/17947/diff/8110/Lib/enum.py#newcode55
Lib/enum.py:55: return self
On 2013/05/10 20:24:06, alex wrote:
> Real properties don't mutate on setter/deleter/getter, but rather
return a new
> instance, I think this should probably mimic that behavior.

There's a getter as well?

http://bugs.python.org/review/17947/diff/8110/Lib/enum.py#newcode68
Lib/enum.py:68: def __setitem__(self, key, something):
On 2013/05/10 20:24:06, alex wrote:
> I think `value` is a more common name to use than `something`.

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py
File Lib/test/test_enum.py (right):

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode54
Lib/test/test_enum.py:54: class Test_Enum(unittest.TestCase):
On 2013/05/10 20:34:34, ezio.melotti wrote:
> TestEnum

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode64
Lib/test/test_enum.py:64: self.assertFalse(_errors)
On 2013/05/10 22:43:10, zach.ware wrote:
> This is better, but now any exceptions that are raised are masked. 
How about
> instead of assertFalse, do (untested):
> """
> for e in _errors:
>     with self.subTest():
>         raise e
> """
> 
> This is also going to cause a lot of NameErrors in the tests that use
these
> Enums, so it would be nice to set dummy values in the except clauses
and skip
> the tests that don't have the necessary Enum available.
> 
> On the other hand, I still think it would be just a whole lot simpler
to lose
> the try/excepts.  If actually creating the Enum is broken, I'd be more
concerned
> with getting that fixed than running the tests that can be run on
non-broken
> Enums.

Saving exceptions and reraising them in the appropriate test (good call,
thanks).

I like that better than having the entire module die, as sometimes
knowing what works is just as valuable as knowing what doesn't when
trying to track down a bug.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode100
Lib/test/test_enum.py:100: self.assertTrue(type(e) is Season)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> assertIs

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode164
Lib/test/test_enum.py:164: self.assertEqual([k for k,v in
Season.__members__.items() if v.name != k], ['FALL', 'ANOTHER_SPRING'])
On 2013/05/10 20:34:34, ezio.melotti wrote:
> Line too long.

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode174
Lib/test/test_enum.py:174: self.assertTrue(type(Huh.name) is Huh)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> assertIs

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode216
Lib/test/test_enum.py:216: self.assertTrue(WeekDay.TEUSDAY is
WeekDay.TUESDAY)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> assertIs

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode218
Lib/test/test_enum.py:218: self.assertEqual([k for k,v in
WeekDay.__members__.items() if v.name != k], ['TEUSDAY', ])
On 2013/05/10 20:34:34, ezio.melotti wrote:
> Line too long.

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode227
Lib/test/test_enum.py:227: self.assertTrue(FloatStooges.CURLY is
loads(dumps(FloatStooges.CURLY)))
On 2013/05/10 20:34:34, ezio.melotti wrote:
> There are more assertTrue that should be converted to assertIs and
others.  In
> general assertTrue/False should be avoided if possible or a message
should be
> added, otherwise in case of failure, a non-informative "False is not
true" will
> be reported.

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode377
Lib/test/test_enum.py:377: self.assertFalse(type(whatever.really) is
whatever)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> assertIsNot

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode389
Lib/test/test_enum.py:389: self.assertFalse(Why.question in Why)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> assertNotIn

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode534
Lib/test/test_enum.py:534: if isinstance( self, NamedInt ) and
isinstance( other, NamedInt ):
On 2013/05/10 20:34:34, ezio.melotti wrote:
> There are extra spaces here.

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode543
Lib/test/test_enum.py:543: y = ('the-y', 2 )
On 2013/05/10 20:34:34, ezio.melotti wrote:
> Here too.

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode556
Lib/test/test_enum.py:556: self.assertTrue(isinstance(SomeTuple.second,
tuple))
On 2013/05/10 20:34:34, ezio.melotti wrote:
> assertIsInstance

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode559
Lib/test/test_enum.py:559:
self.assertTrue(loads(dumps(SomeTuple.first)), SomeTuple.first)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> I think you want an assertIs here too (as it is, SomeTuple.first is
the error
> message).  

Done.

http://bugs.python.org/review/17947/diff/8110/Lib/test/test_enum.py#newcode634
Lib/test/test_enum.py:634: self.assertTrue(Grade.B >= Grade.B)
On 2013/05/10 20:34:34, ezio.melotti wrote:
> You can use assertGreater and friends here.

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py
File Lib/enum.py (right):

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode1
Lib/enum.py:1: """\
On 2013/05/11 05:23:57, eli.bendersky wrote:
> Again, *please* use PEP 257 convention. Even if you don't find the
multi-line
> convention pretty (I personally don't), it's far more important for
the code
> base to be consistently formatted than any particular style.
> 
> One of the best things of Python as an open-source project is the
relative
> consistency of its source code due in terms of style.

Hadn't gotten that far yet.  ;)  Is it good now?

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode13
Lib/enum.py:13: """
On 2013/05/11 05:23:57, eli.bendersky wrote:
> PEP 257 everywhere...
> 
> """First line of multi-line docstring.
> Second line.
> Third line. getting boring. Before the closing triple-quote we have an
empty
> line
> 
> """

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode57
Lib/enum.py:57: return self.__class__(self.fget, self.fset, self.fdel,
self.__doc__)
On 2013/05/11 05:08:27, alex wrote:
> These don't seem to correctly mix the new func in.

Sorry, was on auto-pilot.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode101
Lib/enum.py:101: """\
On 2013/05/11 05:23:57, eli.bendersky wrote:
> """Metaclass for Enum"""

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode115
Lib/enum.py:115: __new__, save_new, use_args =
metacls._find_new(classdict, obj_type, first_enum)
On 2013/05/11 05:23:57, eli.bendersky wrote:
> 80-column violation

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode174
Lib/enum.py:174: def __call__(cls, value, names=None, *, module=None,
type=None):
On 2013/05/11 05:23:57, eli.bendersky wrote:
> Explain the semantics of __call__ in detail in a docstring

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode214
Lib/enum.py:214: return cls._enum_map.copy()
On 2013/05/11 05:23:57, eli.bendersky wrote:
> I still don't like this copy(). Can you say what it is for? If someone
wants to
> screw an Enum up he can access _enum_map directly - Python doesn't
excel at data
> hiding. OTOH, copy needlessly makes innocent code (99.99% of the code)
slower.

Doc string added.  I don't think speed is an issue.  I do think that
innocent user code shouldn't have to worry that removing an item, or
combining with another dict, will corrupt the enumeration.

If they access the private structure it's on them; if they access the
public interface we shouldn't be handing them a time-bomb.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode217
Lib/enum.py:217: """Return the enum item matching `name`"""
On 2013/05/11 05:23:57, eli.bendersky wrote:
> s/item/member/
> 
> Use members consistently everywhere

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode218
Lib/enum.py:218: if name[:2] == name[-2:] == '__':
On 2013/05/11 05:23:57, eli.bendersky wrote:
> You have this more than once in the code. Have a helper method
_is_dundername or
> something

Are you sure?  Seems like this would a much bigger performance hit than
the much rarer call on __members__... and I'm only using it twice.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode223
Lib/enum.py:223: raise AttributeError(name) from None
On 2013/05/11 05:08:27, alex wrote:
> Is there a reason to use __getattr__ over descriptors?

Yup, docstring added.

http://bugs.python.org/review/17947/diff/8113/Lib/enum.py#newcode275
Lib/enum.py:275: # now find the correct __new__, checking to see of one
was defined
On 2013/05/11 05:23:57, eli.bendersky wrote:
> Here and everywhere - use docstrings to provide a method documentation
note.
> Explain what each argument needs.
> 
> Inside the method you can use comments freely

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/test/test_enum.py
File Lib/test/test_enum.py (right):

http://bugs.python.org/review/17947/diff/8113/Lib/test/test_enum.py#newcode9
Lib/test/test_enum.py:9: _errors = []
On 2013/05/11 06:42:08, zach.ware wrote:
> No longer needed.

Done.

http://bugs.python.org/review/17947/diff/8113/Lib/test/test_enum.py#newcode16
Lib/test/test_enum.py:16: Stooges = exc
On 2013/05/11 06:42:08, zach.ware wrote:
> Not quite what I had in mind, but it does seem to work.  But may I ask
what your
> justification for keeping all the try/excepts and now the extra type
checking in
> the tests is?  If you've got a good reason, this method works for me;
I just
> think it could all be a whole lot simpler.

It is entirely possible (and, indeed, it happened to me ;) that only
some of these tests will fail.  If that happens, I still want the rest
to run so we can see what, if anything, is still working.  The whole
point of unit testing is to test everything, not shutdown on the first
failure.

http://bugs.python.org/review/17947/diff/8113/Lib/test/test_enum.py#newcode64
Lib/test/test_enum.py:64: self.assertFalse(_errors)
On 2013/05/11 06:42:08, zach.ware wrote:
> This test is now useless, it can go.

Done.



Please review this at http://bugs.python.org/review/17947/

Affected files:
  Lib/enum.py
  Lib/test/test_enum.py




More information about the docs mailing list