Éric Araujo <mer...@netwok.org> added the comment:
Review time! Please use rietveld for big patches in the future.
I had started with a list of remarks in same order than the code and minor
remarks grouped at the end, but I see now that all my remarks are minor, since
I have found no real code problem (I don’t know pydoc as well as you :) If my
message is hard to follow, I’ll redo it on rietveld or directly update your
patch (I’m nitpicky, so I can volunteer to share the work). Thanks for your
continued effort and high-quality result!
* Misc/NEWS: Your entry has to be proper reST.
* pydoc.rst:
+documentation pages. (The :option:`-g` option is depreciated.)
:option: marks an option for python itself. Use ```` instead. See #9312 for
more info. (Also fix the typo, which appears twice in the diff.)
+:program:`pydoc` ``-b``
:program:`pydoc -b` works.
“Each served page has a navigation bar at the top where you can 'get' help on
an individual item, 'search' all modules with a keyword in their synopsis line,
and goto indexes for 'modules', 'topics' and 'keywords'.”
What do the apostrophes mean?
* urllib/parse.py, test_urlparse.py: Unrelated changes?
* pydoc.py
+import sys, imp, os, re, inspect, builtins, pkgutil, time, warnings
Why not take the opportunity to put each import on its own line, for source and
diff readability?
+ return 'no documentation found for %s' % repr(topic), ''
You can simplify that to “'... %r' % topic”. (I’m assuming topic is never a
tuple.)
+ if type(target) is type(''):
I see you’re following the style of the rest of the file. Modernizing that to
isinstance is probably out of scope for this patch. (Have I said that the HTML
is horrible too?)
+ msg = """The pydoc.serve() function is deprecated."""
Such messages usually don’t start with a capital nor end with a period.
+ msg = """The pydoc.gui() function and "pydoc -g" option are depreciated,
+ use "pydoc.browse() function and "pydoc -b" option instead."""
Won’t this have strange leading indentation on the second line? I’d recommend
printing two lines that wrap under 80 characters:
msg = ('the pydoc.gui() function and "pydoc -g" option are deprecated,'
'use pydoc.browse() and "pydoc -b" instead')
+ #... webbrowser.open(serverthread.url)
+ #True
I don’t like commented-out example code. I prefer it to work or not be there :)
“it's” is not a possessive, you want “its”.
+ import http.server
Aren’t function-level imports frowned upon?
+ if self.path.endswith('.css'):
+ content_type = 'text/css'
+ else:
+ content_type = 'text/html'
It’s good practice to add the “charset” parameter to specify the character
encoding.
+ if url[-5:] == '.html': url = url[:-5]
Please always put two statements on two lines.
+ except IOError as value:
You don’t need to get the instance if you don’t use it in the following block
(IOW, remove “ as value”).
+ fp.close()
Please use the with statement.
+ server_help_msg = """Python Version: %s
“version” need not be capitalized IMO.
-""" % (cmd, os.sep, cmd, cmd, cmd, cmd, os.sep))
+""" % (cmd, os.sep, cmd, cmd, cmd, cmd, cmd, os.sep))
Feel free to replace all those %s by %(cmd)s or {cmd} for readability.
* Assorted nitpicks, which are not blocking:
- “command line” needs an hyphen when used as an adjective (in whatsnew/3.2.rst)
- a→an in “a HTTP server” (pydoc.py)
- Use two spaces: “local machine. Port” (twice); “page. Use”.
- “To determine what the client is asking for check the URL”: I’d add a comma
after “for”.
- You can remove the parentheses around “The "-g" option is deprecated” and put
it on the preceding line.
- Feel free to sprinkle a few more blanklines here and there, especially before
class and function definition.
> Do you agree that the browse function should be public?
I’ll leave that to Nick.
----------
nosy: -BreamoreBoy
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue2001>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
