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

zachary.ware at gmail.com zachary.ware at gmail.com
Mon May 13 19:06:12 CEST 2013


Being mostly satisfied with the state of the test module, I've started
attacking your style ;).  A lot of things I've pointed out here are just
nitpicks, but some are more important (like adding whitespace to
EnumMeta.__new__; it really is intimidating to read as is).


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

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode1
Lib/enum.py:1: """\
It's ok to lose the backslash here, but start the text on line 2.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode8
Lib/enum.py:8: from types import MappingProxyType
It looks better to order by 'import ...', then 'from ... import ...'
rather than intermixing.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode14
Lib/enum.py:14: """ Returns the value in the instance, raises
AtttributeError on the class.
Missed a space before Returns.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode49
Lib/enum.py:49: """ Changes anything not dundered or that doesn't have
__get__.
Missed another space.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode85
Lib/enum.py:85: def __new__(metacls, cls, bases, classdict):
This method really just looks like a giant wall of code to me, not very
readable.  PEP 8 suggests "[Using] blank lines in functions, sparingly,
to indicate logical sections", which I would take to mean roughly before
and/or after each top-level (within the method) for or if statement,
thereabouts.  Or, in this case, before each comment block would seem
appropriate.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode92
Lib/enum.py:92: metacls._find_new(classdict, obj_type, first_enum)
PEP 8 recommends avoiding backslashes for line continuation; it would be
better to split the line in the _find_new call, I think.

"""
        __new__, save_new, use_args = metacls._findnew(classdict,
                                                       obj_type,
                                                       first_enum)
"""

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode224
Lib/enum.py:224: def _create(cls, class_name, names=None, *,
module=None, type=None):
This one could also do with a touch of whitespace to make it more
readable.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode258
Lib/enum.py:258: def _get_mixins(bases):
Whitespace wouldn't be amiss.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode300
Lib/enum.py:300: def _find_new(classdict, obj_type, first_enum):
Another whitespace request.

http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode341
Lib/enum.py:341: """valueless, unordered enumeration class"""
Capitalize "Valueless"

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

http://bugs.python.org/review/17947/diff/8131/Lib/test/test_enum.py#newcode5
Lib/test/test_enum.py:5: import sys
Same comment here as on the enum.py imports.

http://bugs.python.org/review/17947/diff/8131/Lib/test/test_enum.py#newcode100
Lib/test/test_enum.py:100: '<Season.{0}: {1}>'.format(season, i))
PEP 8 violation; this line should either move over to match repr(e), or
repr(e) should move to the next line.

(Nitpicking, ahoy!)

http://bugs.python.org/review/17947/diff/8131/Lib/test/test_enum.py#newcode161
Lib/test/test_enum.py:161: if v.name != k], ['FALL', 'ANOTHER_SPRING'])
This could be broken into separate lines better; the listcomp could
start on the second line, and the list could be on a third line. 
Breaking the listcomp into two lines confused me for a bit when I saw
it.

http://bugs.python.org/review/17947/diff/8131/Lib/test/test_enum.py#newcode192
Lib/test/test_enum.py:192: THURSDAY FRIDAY SATURDAY'''.split(), 1):
I think this might be made more readable by creating a list from the
"SUNDAY MONDAY..." string before the loop statement and passing it in to
enumerate.

http://bugs.python.org/review/17947/diff/8131/Lib/test/test_enum.py#newcode285
Lib/test/test_enum.py:285: self.assertEqual([SummerMonth.june,
SummerMonth.july, SummerMonth.august], lst)
Line too long, there's a few cases of this here and below.

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


More information about the docs mailing list