Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(158028)

#17947: Code, test, and doc review for PEP-0435 Enum

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 7 months ago by ethan
Modified:
6 years, 5 months ago
Reviewers:
eliben, berker.peksag, zachary.ware, merwok, ezio.melotti, alex.gaynor, ncoghlan, mm
CC:
gvanrossum, barry, Nick Coghlan, haypo, Benjamin Peterson, ezio.melotti, alex, eli.bendersky, docs_python.org, stoneleaf, devnull_psf.upfronthosting.co.za, Zach Ware, pconnell, dstufft, isoschiz
Visibility:
Public.

Patch Set 1 #

Total comments: 64

Patch Set 2 #

Total comments: 16

Patch Set 3 #

Total comments: 34

Patch Set 4 #

Total comments: 32

Patch Set 5 #

Total comments: 57

Patch Set 6 #

Total comments: 13

Patch Set 7 #

Patch Set 8 #

Patch Set 9 #

Total comments: 16

Patch Set 10 #

Patch Set 11 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/enum.rst View 1 2 3 4 5 6 7 8 9 10 1 chunk +524 lines, -0 lines 10 comments Download
Lib/enum.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +465 lines, -0 lines 0 comments Download
Lib/test/test_enum.py View 1 2 3 4 5 6 7 8 9 10 1 chunk +836 lines, -0 lines 2 comments Download

Messages

Total messages: 41
eli.bendersky
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 ...
6 years, 7 months ago #1
berkerpeksag
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#newcode17 Lib/enum.py:17: Nit: Two blank lines. (see PEP 8 E302) http://bugs.python.org/review/17947/diff/8105/Lib/enum.py#newcode18 ...
6 years, 7 months ago #2
Zach Ware
Just a few comments on the test module. 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 ...
6 years, 7 months ago #3
eric.araujo
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 I’d just delete the shebang actually, it won’t ...
6 years, 7 months ago #4
ezio.melotti
Added a few comments, mostly about style. 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 ...
6 years, 7 months ago #5
alex
A few small notes (I didn't do a complete review yet) http://bugs.python.org/review/17947/diff/8110/Lib/enum.py File Lib/enum.py (right): ...
6 years, 7 months ago #6
ezio.melotti
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): TestEnum 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) assertIs ...
6 years, 7 months ago #7
Zach Ware
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#newcode64 Lib/test/test_enum.py:64: self.assertFalse(_errors) This is better, but now any exceptions that ...
6 years, 7 months ago #8
alex
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#newcode57 Lib/enum.py:57: return self.__class__(self.fget, self.fset, self.fdel, self.__doc__) These don't seem to ...
6 years, 7 months ago #9
eli.bendersky
Please use the code-review tool to either mark comments "Done" or explain/disagree/discuss, before uploading the ...
6 years, 7 months ago #10
Zach Ware
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 = [] No longer needed. http://bugs.python.org/review/17947/diff/8113/Lib/test/test_enum.py#newcode16 Lib/test/test_enum.py:16: Stooges ...
6 years, 7 months ago #11
stoneleaf
Okay, just discovered the publish and mail portion. This explains why nobody was responding to ...
6 years, 6 months ago #12
stoneleaf
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#newcode244 Lib/enum.py:244: class_name = value # better name for a name ...
6 years, 6 months ago #13
eli.bendersky
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 14:47:00, stoneleaf wrote: > On ...
6 years, 6 months ago #14
eli.bendersky
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 14:47:01, stoneleaf wrote: > On ...
6 years, 6 months ago #15
Nick Coghlan
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() This kind of use case is exactly ...
6 years, 6 months ago #16
eli.bendersky
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: > ...
6 years, 6 months ago #17
Zach Ware
Being mostly satisfied with the state of the test module, I've started attacking your style ...
6 years, 6 months ago #18
isoschiz
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#newcode14 Lib/enum.py:14: """ Returns the value in the instance, raises AtttributeError ...
6 years, 6 months ago #19
alex
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#newcode355 Lib/enum.py:355: return member I would have assumed that that creating ...
6 years, 6 months ago #20
stoneleaf
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 ...
6 years, 6 months ago #21
alex
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#newcode355 Lib/enum.py:355: return member On 2013/05/13 20:32:37, stoneleaf wrote: > On ...
6 years, 6 months ago #22
stoneleaf
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#newcode355 Lib/enum.py:355: return member On 2013/05/13 20:34:50, alex wrote: > On ...
6 years, 6 months ago #23
isoschiz
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/13 20:32:37, ...
6 years, 6 months ago #24
stoneleaf
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 00:01:06, ...
6 years, 6 months ago #25
isoschiz
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 00:09:15, ...
6 years, 6 months ago #26
stoneleaf
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 01:29:56, ...
6 years, 6 months ago #27
isoschiz
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 01:41:45, ...
6 years, 6 months ago #28
stoneleaf
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 20:12:43, ...
6 years, 6 months ago #29
isoschiz
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 20:18:48, ...
6 years, 6 months ago #30
stoneleaf
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#newcode103 Lib/enum.py:103: enum_class = type.__new__(metacls, cls, bases, classdict) On 2013/05/14 20:40:09, ...
6 years, 6 months ago #31
Zach Ware
Here's the latest nitpicks from me :) 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#newcode92 Lib/enum.py:92: metacls._find_new(classdict, obj_type, ...
6 years, 6 months ago #32
alex
http://bugs.python.org/review/17947/diff/8161/Lib/enum.py File Lib/enum.py (right): http://bugs.python.org/review/17947/diff/8161/Lib/enum.py#newcode146 Lib/enum.py:146: enum_class.__module__ = 'uh uh' Can we use a slightly ...
6 years, 6 months ago #33
stoneleaf
http://bugs.python.org/review/17947/diff/8161/Lib/enum.py File Lib/enum.py (right): http://bugs.python.org/review/17947/diff/8161/Lib/enum.py#newcode7 Lib/enum.py:7: import weakref On 2013/05/17 15:37:11, zach.ware wrote: > As ...
6 years, 6 months ago #34
Zach Ware
http://bugs.python.org/review/17947/diff/8161/Lib/enum.py File Lib/enum.py (right): http://bugs.python.org/review/17947/diff/8161/Lib/enum.py#newcode123 Lib/enum.py:123: "'_find_new' cannot be used for members") On 2013/05/17 17:40:14, ...
6 years, 6 months ago #35
eli.bendersky
http://bugs.python.org/review/17947/diff/8332/Doc/library/enum.rst File Doc/library/enum.rst (right): http://bugs.python.org/review/17947/diff/8332/Doc/library/enum.rst#newcode240 Doc/library/enum.rst:240: of *_sunder_* names, *__dunder__* names, and descriptors; methods are ...
6 years, 6 months ago #36
stoneleaf
I'll try to have the changes in place later tonight; otherwise it will be tomorrow. ...
6 years, 6 months ago #37
Zach Ware
A few nitpicks and a suggestion. Everything else looks good to me. http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst File Doc/library/enum.rst ...
6 years, 6 months ago #38
stoneleaf
http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst File Doc/library/enum.rst (right): http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst#newcode60 Doc/library/enum.rst:60: Enums also have a property that contains just their ...
6 years, 6 months ago #39
stoneleaf
http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst File Doc/library/enum.rst (right): http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst#newcode292 Doc/library/enum.rst:292: Added: .. warning:: In order to support the singleton ...
6 years, 5 months ago #40
Zach Ware
6 years, 5 months ago #41
Barring any typos in the not-yet-uploaded v12 patch, everything looks good to me
now.  Thanks, Ethan!

http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst
File Doc/library/enum.rst (right):

http://bugs.python.org/review/17947/diff/8339/Doc/library/enum.rst#newcode423
Doc/library/enum.rst:423: One frequent request is to not have to specify values
for enum members::
On 2013/06/10 16:13:01, stoneleaf wrote:
> Major rewrite:

Much better :)
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+