Author eric.araujo
Recipients belopolsky, eric.araujo, georg.brandl, giampaolo.rodola, ncoghlan, ping, r.liebscher, ron_adam, srid
Date 2010-11-18.04:04:29
SpamBayes Score 1.11022e-16
Marked as misclassified No
Message-id <1290053071.99.0.624073176454.issue2001@psf.upfronthosting.co.za>
In-reply-to
Content
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.
History
Date User Action Args
2010-11-18 04:04:32eric.araujosetrecipients: + eric.araujo, ping, georg.brandl, ncoghlan, belopolsky, giampaolo.rodola, ron_adam, r.liebscher, srid
2010-11-18 04:04:31eric.araujosetmessageid: <1290053071.99.0.624073176454.issue2001@psf.upfronthosting.co.za>
2010-11-18 04:04:29eric.araujolinkissue2001 messages
2010-11-18 04:04:29eric.araujocreate