Issue 3202042: pydoc.Helper.help() ignores input/output init parameters (issue 940286)
Patch Set 1 #
Total comments: 12
Patch Set 2 : changes based on merwok's comments #
Total comments: 2
Patch Set 3 : cleaning up the unit test #
Total comments: 6
Patch Set 4 : code style fixes #
Total comments: 1
MessagesTotal messages: 9
The diff is short and simple, but introduces new arguments in some methods, which makes me uneasy (we try to fix bugs in stable versions without adding features). Is it possible to fix the behavior without adding arguments? http://codereview.appspot.com/3202042/diff/1/Lib/pydoc.py File Lib/pydoc.py (right): http://codereview.appspot.com/3202042/diff/1/Lib/pydoc.py#newcode1313 Lib/pydoc.py:1313: class PlainTextDoc(TextDoc): I would make that a private class (_PlainTextDoc). http://codereview.appspot.com/3202042/diff/1/Lib/pydoc.py#newcode1314 Lib/pydoc.py:1314: """Subclass of TextDoc which overrides string formatting""" Using “string formatting” is confusing here: this term is used to refer to %-formatting or {}-formatting, not using terminal escape sequences for styling. I don’t have a better wording right now. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py File Lib/test/test_pydoc.py (right): http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode336 Lib/test/test_pydoc.py:336: global expected_text_pattern No need for global here. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode342 Lib/test/test_pydoc.py:342: buf = StringIO() You should also check that nothing is printed to stdout or stderr (use test.support.captured_output) http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode349 Lib/test/test_pydoc.py:349: """.lstrip().rstrip(' ') Using textwrap.dedent may be clearer here. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode360 Lib/test/test_pydoc.py:360: if result != expected_text: Do you have to do that by hand? Isn’t assertEqual good enough? (Hint: you may have to set “maxDiff = None” on the test class)
Sign in to reply to this message.
A couple of thoughts on the new arguments I've added: 1.) An alternative approach to injecting dependencies along the Helper->doc->render_doc->_PlainTextDoc execution path would be to render it directly from Helper using an 'if self._output'. Doing this, however, would mean giving up all the nice error checking and conditions checked in all of these functions. 2.) The arguments I've added are backwards compatible, have reasonable default arguments, and affect functions which were only meant for use inside the pydoc module. I am open to an alternative implementation, if a better solution comes up. http://codereview.appspot.com/3202042/diff/1/Lib/pydoc.py File Lib/pydoc.py (right): http://codereview.appspot.com/3202042/diff/1/Lib/pydoc.py#newcode1313 Lib/pydoc.py:1313: class PlainTextDoc(TextDoc): Agreed On 2010/11/21 22:46:15, merwok wrote: > I would make that a private class (_PlainTextDoc). http://codereview.appspot.com/3202042/diff/1/Lib/pydoc.py#newcode1314 Lib/pydoc.py:1314: """Subclass of TextDoc which overrides string formatting""" I have replaced "formatting" with "styling" On 2010/11/21 22:46:15, merwok wrote: > Using “string formatting” is confusing here: this term is used to refer to > %-formatting or {}-formatting, not using terminal escape sequences for styling. > I don’t have a better wording right now. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py File Lib/test/test_pydoc.py (right): http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode336 Lib/test/test_pydoc.py:336: global expected_text_pattern Test throws "UnboundLocalError: local variable 'expected_text_pattern' referenced before assignment" without the global. On 2010/11/21 22:46:15, merwok wrote: > No need for global here. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode342 Lib/test/test_pydoc.py:342: buf = StringIO() Agreed. On 2010/11/21 22:46:15, merwok wrote: > You should also check that nothing is printed to stdout or stderr (use > test.support.captured_output) http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode349 Lib/test/test_pydoc.py:349: """.lstrip().rstrip(' ') Agreed. On 2010/11/21 22:46:15, merwok wrote: > Using textwrap.dedent may be clearer here. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode360 Lib/test/test_pydoc.py:360: if result != expected_text: I am using test_html_doc and test_text_doc as a template for my test. It keeps to the style set down by the previous tests and has the advantage of a nice unified diff. On 2010/11/21 22:46:15, merwok wrote: > Do you have to do that by hand? Isn’t assertEqual good enough? (Hint: you may > have to set “maxDiff = None” on the test class)
Sign in to reply to this message.
> A couple of thoughts on the new arguments I've added: [...] Okay, I’m convinced. Lib/pydoc.py:1314: """Subclass of TextDoc which overrides string formatting""" > I have replaced "formatting" with "styling" Good enough for a docstring in a private class. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode336 Lib/test/test_pydoc.py:336: global expected_text_pattern > Test throws "UnboundLocalError: local variable > 'expected_text_pattern' referenced before assignment" > without the global. Ah, I hadn’t seen that you were binding the name later in the file. I’m not sure it’s a good idea to use globals in tests. http://codereview.appspot.com/3202042/diff/1/Lib/test/test_pydoc.py#newcode360 Lib/test/test_pydoc.py:360: if result != expected_text: > I am using test_html_doc and test_text_doc as a template > for my test. It keeps to the style set down by the > previous tests and has the advantage of a nice unified diff. assertEqual should do the diff too, that’s why I asked. You are not forced to follow the style of other tests; maybe they were written before TestCase had diff possibility, so you can use it. The advantage of using standard unittest features is that it removes custom code in test; removing code eases maintenance and does not force us to test test code :) It’s not really important anyway. (Your message was a bit hard to read, especially considering that your replies are *before* the questions.) http://codereview.appspot.com/3202042/diff/6001/Lib/test/test_pydoc.py File Lib/test/test_pydoc.py (right): http://codereview.appspot.com/3202042/diff/6001/Lib/test/test_pydoc.py#newcod... Lib/test/test_pydoc.py:355: with captured_stdout() as output: Just in case, you should check stderr too.
Sign in to reply to this message.
> Ah, I hadn’t seen that you were binding the name later in the file. I’m not > sure it’s a good idea to use globals in tests. I have refactored out the global statement. > assertEqual should do the diff too, that’s why I asked. You are not forced to > follow the style of other tests; maybe they were written before TestCase had > diff possibility, so you can use it. The advantage of using standard unittest > features is that it removes custom code in test; removing code eases maintenance > and does not force us to test test code :) It’s not really important anyway. You've convinced me, I've changed it to an assertEquals. > (Your message was a bit hard to read, especially considering that your replies > are *before* the questions.) Oops! Thanks for pointing that out, I'm a little new to the tools and process, so this kind of stuff wasn't obvious to me. http://codereview.appspot.com/3202042/diff/6001/Lib/test/test_pydoc.py File Lib/test/test_pydoc.py (right): http://codereview.appspot.com/3202042/diff/6001/Lib/test/test_pydoc.py#newcod... Lib/test/test_pydoc.py:355: with captured_stdout() as output: On 2010/11/22 13:51:40, merwok wrote: > Just in case, you should check stderr too. Agreed
Sign in to reply to this message.
>> Ah, I hadn’t seen that you were binding the name later in the file. > I’m not Yes you were :) → expected_text_pattern = help_header + expected_text_pattern > You've convinced me, I've changed it to an assertEquals. This is actually a deprecated alias, I’ll change it to assertEqual before committing. >> (Your message was a bit hard to read, especially considering that your replies >> are *before* the questions.) > Oops! Thanks for pointing that out, I'm a little new to the tools > and process, so this kind of stuff wasn't obvious to me. It’s not obvious for anyone, so don’t worry :)
Sign in to reply to this message.
Nothing of substance, just a few whitespace issues. http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py File Lib/pydoc.py (right): http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py#newcode1315 Lib/pydoc.py:1315: def bold(self,text): Please add space after comma. http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py#newcode1485 Lib/pydoc.py:1485: def render_doc(thing, title='Python Library Documentation: %s', forceload=0, renderer=None): please keep lines under 79 characters. http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py#newcode1509 Lib/pydoc.py:1509: def doc(thing, title='Python Library Documentation: %s', forceload=0, output=None): long line again http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py#newcode1768 Lib/pydoc.py:1768: elif request: doc(request, 'Help on %s:',output=self._output) no space after comma http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py#newcode1770 Lib/pydoc.py:1770: else: doc(request, 'Help on %s:',output=self._output) no space after comma
Sign in to reply to this message.
http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py File Lib/pydoc.py (right): http://codereview.appspot.com/3202042/diff/11001/Lib/pydoc.py#newcode1512 Lib/pydoc.py:1512: if output: better "if output is not None" or even better "if output is None" and swap the clauses.
Sign in to reply to this message.
> >> Ah, I hadn’t seen that you were binding the name later in the file. > > I’m not > Yes you were :) > → expected_text_pattern = help_header + expected_text_pattern I think my message didn't come across correctly. I said "I have refactored out the global statement." Agreeing that I should remove the global. The "I'm not" came from quoting your message "I’m not sure it’s a good idea to use globals in tests". There should be some better way to handle quotes :/ > > You've convinced me, I've changed it to an assertEquals. > This is actually a deprecated alias, I’ll change it to assertEqual before > committing. I'll fix this.
Sign in to reply to this message.
A last style remark. The rest is good to go. http://codereview.appspot.com/3202042/diff/19001/Lib/test/test_pydoc.py File Lib/test/test_pydoc.py (right): http://codereview.appspot.com/3202042/diff/19001/Lib/test/test_pydoc.py#newco... Lib/test/test_pydoc.py:364: finally: I prefer using self.addCleanup(setattr, pydoc, 'getpager', pydoc.getpager) at the start of the method. It’s cleaner to read than a try/finally and removes the need to keep around references to monkey-patched objects.
Sign in to reply to this message.
|
