[issue2001] Pydoc interactive browsing enhancement

[Éric Araujo]

Éric Araujo <merwok at 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 <report at bugs.python.org>
<http://bugs.python.org/issue2001>
_______________________________________