Python #8525

[merwok, 2010-11-22 00:25:37]

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py
File Lib/pydoc.py (right):

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1145
Lib/pydoc.py:1145: if not attrval is object:
If find it more natural to read “is not”.

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1151
Lib/pydoc.py:1151: for i in range(len(subclasses)-1, -1, -1):
It’s possible that I’m too tired to parse the logic here, but if there is a way
to make that clearer, it would be nice.  Otherwise don’t bother.

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1158
Lib/pydoc.py:1158: for subclassname in sorted([cls.__name__ for cls in
subclasses]):
Micro-optimization: Don’t build an intermediate list, use a generator
expression: sorted(cls.__name__ for cls in subclasses)

http://codereview.appspot.com/3169042/diff/1/Lib/test/test_pydoc.py
File Lib/test/test_pydoc.py (right):

http://codereview.appspot.com/3169042/diff/1/Lib/test/test_pydoc.py#newcode331
Lib/test/test_pydoc.py:331: """Tests help on builtin object which have only
child classes.
Test methods don’t have docstrings.  They may have a one-line comment that is
printed during the test run, but it’s not needed if the name is clear enough. 
Could you add information to the reST documentation and/or a docstring?

[sasha, 2010-11-22 01:25:38]

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py
File Lib/pydoc.py (right):

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1144
Lib/pydoc.py:1144: for attr, attrval in __builtins__.items():
"attr" is not used below.  I would rewrite the loop using

for builtin in __builtins__.values()

Also, while it is unlikely that pydoc will ever override __builtins__, it may be
cleaner to use the builtins module here.

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1147
Lib/pydoc.py:1147: if issubclass(attrval, object):
Why not use object.__subclasses__() and check __module__?

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1152
Lib/pydoc.py:1152: if issubclass(subclasses[i],
tuple(subclasses[:i]+subclasses[i+1:])):
I believe that using object.__subclasses__() will eliminate the need for this.

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1155
Lib/pydoc.py:1155: # i.e. keep only immediate child classes of "object", not
grandchildren etc.
Please keep lines under 79 columns.

[merwok, 2010-11-22 01:46:06]

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py
File Lib/pydoc.py (right):

http://codereview.appspot.com/3169042/diff/1/Lib/pydoc.py#newcode1144
Lib/pydoc.py:1144: for attr, attrval in __builtins__.items():
It is always correct to use builtins instead of the CPython implementation
detail.