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

eliben at gmail.com eliben at gmail.com
Fri May 10 16:02:43 CEST 2013


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: =========
Yeah, remove this :-)

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode19
Lib/enum.py:19: """
PEP257-ify

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
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.

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__'):
parenthesize the parts of the or

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode85
Lib/enum.py:85: if key in self._enum_names:
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

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode92
Lib/enum.py:92: dict.__setitem__(self, key, something)
Perhaps using super() here would be cleaner and more future-proof

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode94
Lib/enum.py:94: Enum = None  # dummy value until replaced
beefier comment that explains why this dummy is needed. Also, would this
need a docstring? I'm thinking of 

>> import enum
>> help(enum.Enum)

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode97
Lib/enum.py:97: class EnumMeta(type):
Should this be _EnumMeta too?

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode98
Lib/enum.py:98: """Metaclass for Enum
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

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode100
Lib/enum.py:100: Pure enumerations can take any value, but the
enumeration item will not
It's not clear what a "pure" enum is

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode107
Lib/enum.py:107: resulting psuedenums must all be of that mixed-in type.
psuedenums?

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;
Explain in more detail what a "data type" is

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
Hmm, this __new__ protocol has to be documented on a higher level first
(in the docs?)

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode127
Lib/enum.py:127: and issubclass(base, Enum)
indentation

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode130
Lib/enum.py:130: if not issubclass(base, Enum):
make it more explicit that base here is the last base!

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,
theres no mixin_type in the code. also close the paren )

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode159
Lib/enum.py:159: if __new__ is None:
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

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):
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.

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
;)
I'd suggest a new method here:

_make_enum_from_names(name, member_names, ....)

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode280
Lib/enum.py:280: return cls._enum_map.copy()
why copy?

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode318
Lib/enum.py:318: enum_member = cls._enum_map[enum_name]
why don't you just search enum_map directly?

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode344
Lib/enum.py:344: @StealthProperty
OK, here a brief explanation of how _StealthProperty is actually used
*in Enum* and why

http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode356
Lib/enum.py:356: if __name__ == '__main__':
remove this

http://bugs.python.org/review/17947/


More information about the docs mailing list