[docs] Code, test, and doc review for PEP-0435 Enum (issue 17947)
ethan at stoneleaf.us
ethan at stoneleaf.us
Mon May 13 20:32:37 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#newcode18
Lib/enum.py:18: class StealthProperty():
On 2013/05/12 15:02:00, eli.bendersky wrote:
> On 2013/05/12 14:47:00, stoneleaf wrote:
> > 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.
>
> Remove them - they're not needed in Python 3 and their presence is
confusing.
Done.
http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode280
Lib/enum.py:280: return cls._enum_map.copy()
On 2013/05/12 15:02:00, eli.bendersky wrote:
> On 2013/05/12 14:47:01, stoneleaf wrote:
> > 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).
>
> That's tough life, no doubt ;-), but appropriate in Python. You can do
much
> worse things to objects and classes - we don't do safeguards here.
Consenting
> adults.
That's not actually correct. We don't do many, but we do do some. dict
is a good example of a class with safeguards (which I just learned from
a PyCon 2013 talk by Raymond, I believe).
Furthermore, the whole Enum concept is based in part on safeguards, else
why the fuss about Animal instances not comparing equal to Shape
instances?
Plus, I'm not disallowing access to _enum_map -- if a user really wants
to, one can get to it and do whatever one wants for good or ill --
that's what consenting adults means to me... not "here's your drink, but
don't eat the ice, they're poisoned". ;)
What it comes down to for me is, is the performance benefit of a rarely
called property worth the inevitable bugs caused by returning the actual
internal data structure?
The answer, again for me, is No.
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#newcode214
Lib/enum.py:214: return cls._enum_map.copy()
On 2013/05/12 15:28:03, Nick Coghlan wrote:
> This kind of use case is exactly why types.MappingProxyType was made
visible at
> the Python layer (it's the impl of the read-only mappings used for
class
> dictionaries, and we use them there for a similar reason: modifying
the class
> contents without going through the class machinery simply isn't
something we
> want to allow).
>
> So, rather than copying, I suggest returning
> types.MappingProxyType(cls._enum_map)
>
> http://docs.python.org/3/library/types#types.MappingProxyType
Thanks, Nick -- that I can do.
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: """\
On 2013/05/13 19:06:12, zach.ware wrote:
> It's ok to lose the backslash here, but start the text on line 2.
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode8
Lib/enum.py:8: from types import MappingProxyType
On 2013/05/13 19:06:12, zach.ware wrote:
> It looks better to order by 'import ...', then 'from ... import ...'
rather than
> intermixing.
Done.
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.
On 2013/05/13 19:06:12, zach.ware wrote:
> Missed a space before Returns.
Done.
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.
On 2013/05/13 19:25:49, isoschiz wrote:
> (minor) typo in "AtttributeError"
Done.
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__.
On 2013/05/13 19:06:12, zach.ware wrote:
> Missed another space.
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode56
Lib/enum.py:56: error is raised.
On 2013/05/13 19:25:49, isoschiz wrote:
> This sentence is not true (though it might be nicer if it was! i.e.
sometimes it
> might be easier when using the Functional API to just leave duplicates
in than
> round trip via an "OrderedSet").
Forgot to update the docstring. Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode85
Lib/enum.py:85: def __new__(metacls, cls, bases, classdict):
On 2013/05/13 19:06:12, zach.ware wrote:
> 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.
I was wondering if I could add some blank lines. Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode92
Lib/enum.py:92: metacls._find_new(classdict, obj_type, first_enum)
On 2013/05/13 19:06:12, zach.ware wrote:
> 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)
> """
How about:
"""
__new__, save_new, use_args = (
metacls._find_new(classdict, obj_type, first_enum)
)
"""
?
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode103
Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases,
classdict)
On 2013/05/13 19:25:49, isoschiz wrote:
> Shouldn't this technically be a use of super()? Not that I guess
complex type
> hierarchies of Metaclasses are common.
__new__ is one of the few things (the only thing?) that cannot be shared
-- you can only create an object once.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode114
Lib/enum.py:114: if not isinstance(value, tuple):
On 2013/05/13 19:25:49, isoschiz wrote:
> Doesn't this produce weird behaviour for the following definition:
>
> class MyEnum(Enum):
> FOO = 7
> BAR = (42, 8)
> BAZ = "hello"
Did you try it?
"""
--> class MyEnum(Enum): FOO = 7; BAR = (42, 8); BAZ = "hello"
...
--> MyEnum
<enum 'MyEnum'>
--> list(MyEnum)
[<MyEnum.FOO: 7>, <MyEnum.BAR: (42, 8)>, <MyEnum.BAZ: 'hello'>]
--> MyEnum.BAR
<MyEnum.BAR: (42, 8)>
--> MyEnum.BAR.value == (42, 8)
True
"""
Looks good to me. ;)
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode123
Lib/enum.py:123: enum_item = __new__(enum_class, *args)
On 2013/05/13 19:25:49, isoschiz wrote:
> Is it explicitly forbidden to have multi type enums (i.e. ones where
different
> items have different values)? This code assumes that the __new__ from
the first
> type can cope with the args from the latter ones. I believe the
__new__ needs to
> be re-calculated on each enum item in order to correctly cope.
If it's a mixed enum (like IntEnum) all the values must be of the mixed
in type; if it's a plain Enum, object.__new__ is used and the value set
afterwards.
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):
On 2013/05/13 19:06:12, zach.ware wrote:
> This one could also do with a touch of whitespace to make it more
readable.
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode258
Lib/enum.py:258: def _get_mixins(bases):
On 2013/05/13 19:06:12, zach.ware wrote:
> Whitespace wouldn't be amiss.
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode300
Lib/enum.py:300: def _find_new(classdict, obj_type, first_enum):
On 2013/05/13 19:06:12, zach.ware wrote:
> Another whitespace request.
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode323
Lib/enum.py:323: Enum.__new__):
On 2013/05/13 19:25:49, isoschiz wrote:
> This should be a set literal, not a tuple.
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode341
Lib/enum.py:341: """valueless, unordered enumeration class"""
On 2013/05/13 19:06:12, zach.ware wrote:
> Capitalize "Valueless"
Done.
http://bugs.python.org/review/17947/diff/8131/Lib/enum.py#newcode355
Lib/enum.py:355: return member
On 2013/05/13 19:35:53, alex wrote:
> I would have assumed that that creating an enum from a value would be
O(1), not
> O(n), is there no way we can improve this?
Sure, but it would add a bunch of complexity, and is it worth it for a
one time operation?
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
On 2013/05/13 19:06:12, zach.ware wrote:
> Same comment here as on the enum.py imports.
Done.
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))
On 2013/05/13 19:06:12, zach.ware wrote:
> PEP 8 violation; this line should either move over to match repr(e),
or repr(e)
> should move to the next line.
>
> (Nitpicking, ahoy!)
Done.
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'])
On 2013/05/13 19:06:12, zach.ware wrote:
> 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.
Done.
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):
On 2013/05/13 19:06:12, zach.ware wrote:
> 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.
Done.
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)
On 2013/05/13 19:06:12, zach.ware wrote:
> Line too long, there's a few cases of this here and below.
Done.
http://bugs.python.org/review/17947/
More information about the docs
mailing list