Replace usage of twisted.python.text in twisted.lore

[Thijs Triemstra]

twisted.python.text is being deprecated (#6341) and its usage should be replaced in twisted.lore.

[Thijs Triemstra]

(In [38632]) Branching to 'replace-text-6546'

[Thijs Triemstra]

(In [38633]) replace usage of t.python.text, refs #6546

[Thijs Triemstra]

Replaced the usage in twisted.lore. Added a copy of removeLeadingTrailingBlanks and removeLeadingTrailingBlanks from twisted.python.text to twisted.lore.tree because lore was the only user of them.

Forced a ​build.

[Richard Wall]

Code Review

Hi thijs,

1. branches/replace-text-6546/twisted/lore/slides.py
   1. Why split "import os.path, re" onto separate lines? I can't find it in the coding standard. (Same comment applies to other files in this patch too.)
   2. Add a docstring for the "visitNode" function.
2. branches/replace-text-6546/twisted/lore/test/test_lore.py
   1. The new "RemoveBlanksTests" seem demonstrate that tree._removeLeadingBlanks does nothing to the input and it looks like the same is true of t.p.text.removeLeadingBlanks too! Or am I misunderstanding?
   2. And "removeLeadingTrailingBlanks" only seems to add a trailing new line to the input without stripping any whitespace. Same goes for t.p.text.removeLeadingTrailingBlanks.
3. twisted/lore/test/test_texi.py
   1. The variable name "pre" in "test_title" is confusing.
4. branches/replace-text-6546/twisted/lore/test/test_latex.py
   1. These new lore tests look worthwhile, but they don't seem to be related to the removal of t.p.text. Why have you added them here? Are you just adding coverage for the functions which this branch changes?
5. branches/replace-text-6546/twisted/lore/test/test_slides.py
   1. Minor nit - I don't like the way the long import line has been wrapped. I'd prefer:

      twisted.lore.slides import (
          HTMLSlide, splitIntoSlides, insertPrevNextLinks, MagicpointOutput)
      

6. branches/replace-text-6546/twisted/lore/tree.py
   1. As far as I can see the two text functions never did anything useful. The original (unstripped) line is returned.

       if ret or line.strip():
           ret.append(line)
      

   2. I think you should just remove the calls to those two functions and add a blank line wherever "removeLeadingTrailingBlanks" was used.

In [1]: from twisted.python import text

In [2]: from twisted.lore import tree

In [3]: input = """
   ...:  foo
   ...:   bar
   ...:    baz  """

In [4]: text.remo
text.removeLeadingBlanks          text.removeLeadingTrailingBlanks

In [4]: text.removeLeadingTrailingBlanks(input)
Out[4]: ' foo\n  bar  \n   baz  \n'

In [5]: print text.removeLeadingTrailingBlanks(input)
 foo
  bar
   baz


In [6]: print tree._remo(input)
tree._removeLeadingBlanks          tree._removeLeadingTrailingBlanks

In [6]: tree._removeLeadingTrailingBlanks(input)
Out[6]: ' foo\n  bar  \n   baz  \n'

Please resubmit for review after addressing or answering the points above.

Thanks again.

-RichardW.

[Thijs Triemstra]

(In [38910]) address review coments, refs #6546

[Thijs Triemstra]

Thanks for your review Richard.

Replying to rwall:

1. branches/replace-text-6546/twisted/lore/slides.py
   1. Why split "import os.path, re" onto separate lines? I can't find it in the coding standard. (Same comment applies to other files in this patch too.)

You're right, it's not in the coding standard, I'm not sure where i picked this up, probably some code review, twistedcheckers warning or pep8 thing.. Reverted these changes.

2. Add a docstring for the "visitNode" function.

lore.slides doesn't contain a visitNode function as such, if you're referring to the method i modified (visitNode_pre), I'm not sure if I should document it because 1) I'm not sure what it *exactly* does and 2) if I document that method I kinda need to document them all and that seems overkill for this ticket 3) lore is hardly maintained. Thoughts?

2. branches/replace-text-6546/twisted/lore/test/test_lore.py
   1. The new "RemoveBlanksTests" seem demonstrate that tree._removeLeadingBlanks does nothing to the input and it looks like the same is true of t.p.text.removeLeadingBlanks too! Or am I misunderstanding?

lore is the only user of t.p.text.removeLeadingBlanks and removeLeadingTrailingBlanks so I copied them over to lore as private helpers. Because I don't want to mess too much with lore (seems nobody does) this is the approach I took, and made sure changes to lore were covered by tests. I agree the methods are weird but it is what it is. I'm a bit hesitant to start cleaning up things in lore (and make the changes you suggest in point 6) before hearing your thoughts on this.

2. And "removeLeadingTrailingBlanks" only seems to add a trailing new line to the input without stripping any whitespace. Same goes for t.p.text.removeLeadingTrailingBlanks.

3. twisted/lore/test/test_texi.py
   1. The variable name "pre" in "test_title" is confusing.

Renamed.

4. branches/replace-text-6546/twisted/lore/test/test_latex.py
   1. These new lore tests look worthwhile, but they don't seem to be related to the removal of t.p.text. Why have you added them here? Are you just adding coverage for the functions which this branch changes?

Yep. And that's true for all changes here, to add coverage where necessary, I'm not trying to change/enhance lore, no :) its part of getting the parent ticket resolved.

5. branches/replace-text-6546/twisted/lore/test/test_slides.py
   1. Minor nit - I don't like the way the long import line has been wrapped. I'd prefer:

      twisted.lore.slides import (
          HTMLSlide, splitIntoSlides, insertPrevNextLinks, MagicpointOutput)
      

Fixed.

[Richard Wall]

Replying to thijs:

lore is the only user of t.p.text.removeLeadingBlanks and removeLeadingTrailingBlanks so I copied them over to lore as private helpers. Because I don't want to mess too much with lore (seems nobody does) this is the approach I took, and made sure changes to lore were covered by tests. I agree the methods are weird but it is what it is. I'm a bit hesitant to start cleaning up things in lore (and make the changes you suggest in point 6) before hearing your thoughts on this.

I kind of see your point but personally I'd remove those functions.

They clearly don't do anything so what's the point in leaving them to cause further confusion later on.

Maybe add a test or two to clarify the expected current behaviour - that those strings *are not* stripped and that a new line is appended.

The other changes look good and I obviously got confused about the name of visitNode_pre function - ignore my comment about documenting it.

I'll leave the ticket in review so that someone else can comment.

I forced a new build on that branch:

• ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/replace-text-6546&num_builds=5

-RichardW.

[Tom Prince]

Thanks for your work on this.

if I document that method I kinda need to document them all and that seems overkill for this ticket

No. It is reasonable to just document changed methods.

1. Regarding the behavior of the functions, the names are somewhat misleading. They remove blank lines (or lines consisting of entirely of whitespace) at the begining and end of the string.
2. The test for LatexSplitter.visitNode_code doesn't seem to test any of the interesting behavior of the method. (It seems to be something about inserting optional linebreaks at sensible places in absolute python names)
3. Similarily, the test for MagicPointOutput.visitNode_body doesn't test any of the interesting behavior. (And the one for visit_pre doesn't handle the case of multiple lines.
4. The same is true for TexiSplitter.visit_title.
5. I guess 2-3 are mostly about code near changes, rather than actually changed. I guess if the docstring were changed to indicate what case of the function they are testing, that would be sufficient. One thing they don't appear to test, though, is that removeLeadingTrailingBlanks is actually called.

Please resubmit for review after addressing 1+5.

[Richard Wall]

Replying to tom.prince:

1. Regarding the behavior of the functions, the names are somewhat

misleading. They remove blank lines (or lines consisting of entirely of whitespace) at the beginning and end of the string.

Sorry I didn't spot that.

But the function names are confusing. How about:

_removeLeadingBlankLines _removeLeadingTrailingBlankLines

And the new docstrings for those functions are misleading. I think they should certainly be changed.

[Thijs Triemstra]

Replying to tom.prince:

if I document that method I kinda need to document them all and that seems overkill for this ticket

No. It is reasonable to just document changed methods.

Added docstrings with a type of xml.dom.minidom.Element for node, not sure if that's always true though..

1. Regarding the behavior of the functions, the names are somewhat misleading. They remove blank lines (or lines consisting of entirely of whitespace) at the begining and end of the string.

Changed the names as suggested by rwall and updated the tests and docstrings.

2. The test for LatexSplitter.visitNode_code doesn't seem to test any of the interesting behavior of the method. (It seems to be something about inserting optional linebreaks at sensible places in absolute python names)

Expanded the test.

3. Similarily, the test for MagicPointOutput.visitNode_body doesn't test any of the interesting behavior. (And the one for visit_pre doesn't handle the case of multiple lines.

Expanded the test.

4. The same is true for TexiSplitter.visit_title.

Expanded the test.

5. I guess 2-3 are mostly about code near changes, rather than actually changed. I guess if the docstring were changed to indicate what case of the function they are testing, that would be sufficient. One thing they don't appear to test, though, is that removeLeadingTrailingBlanks is actually called.

LatexSplitter.visitNode_code mentioned in point 2 doesn't call removeLeadingTrailingBlanks but LatexSplitter.visitNode_pre does. I added a few more leading and trailing newlines in that test which show that it's called because '\n\n\nfoo\nbar\n\n\n' becomes 'foo\nbar\n'.

Same for MagicPointOutput.visitNode_pre; it now also uses an input value containing leading and trailing newlines that show _removeLeadingTrailingBlankLines is called.

Thanks for the review, forced a new ​build.

[Richard Wall]

Thanks Thijs,

This all looks good to me. The new function names and added docstrings will avoid any confusion in the future and the new tests all make sense.

I won't claim to understand all the markup that is being tested though.

Notes:

• Merges cleanly
• New tests all pass
• Much improved coverage

   [richard@zorin Twisted]$ diff --unified=0 trunk/coverage.report branches/replace-text-6546/coverage.report
   --- trunk/coverage.report	2013-09-10 10:52:33.307704686 +0100
   +++ branches/replace-text-6546/coverage.report	2013-09-10 10:52:42.380674910 +0100
   @@ -4 +4 @@
   -twisted/lore/_version                 3      0   100%
   +twisted/lore/_version                 2      0   100%
   @@ -9 +9 @@
   -twisted/lore/latex                  318    138    57%
   +twisted/lore/latex                  318    128    60%
   @@ -17 +17 @@
   -twisted/lore/slides                 227    135    41%
   +twisted/lore/slides                 226     88    61%
   @@ -20 +20 @@
   -twisted/lore/test/test_latex         66      0   100%
   +twisted/lore/test/test_latex         80      0   100%
   @@ -23 +23 @@
   -twisted/lore/test/test_lore         450     14    97%
   +twisted/lore/test/test_lore         459     14    97%
   @@ -26,3 +26,4 @@
   -twisted/lore/test/test_slides        49      0   100%
   -twisted/lore/texi                    79     79     0%
   -twisted/lore/tree                   382     29    92%
   +twisted/lore/test/test_slides        84      0   100%
   +twisted/lore/test/test_texi          42      0   100%
   +twisted/lore/texi                    78      6    92%
   +twisted/lore/tree                   394     29    93%
   @@ -30 +31 @@
   -TOTAL                              2448    602    75%
   +TOTAL                              2557    472    82%
  

Some minor cosmetic points:

1. Build Failure
   1. Python3 -- I think that failure is unrelated and has now been fixed in trunk so we can ignore it.
   2. TwistedChecker: ​https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1122/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

      ************* Module twisted.lore.test.test_latex
      C0301:102,0: Line too long (83/79)
      

2. source:branches/replace-text-6546/twisted/lore/slides.py
   1. C{xml.dom.minidom.Element} >> L{}
3. source:branches/replace-text-6546/twisted/lore/latex.py
   1. C{xml.dom.minidom.Element} >> L{}
4. source:branches/replace-text-6546/twisted/lore/tree.py
   1. @type lines: L{str} >> L{list} (I think)
5. source:branches/replace-text-6546/twisted/lore/texi.py
   1. C{xml.dom.minidom.Element} >> L{}

Please merge after addressing the points above and checking for clean build results.

-RichardW.

[Thijs Triemstra]

(In [39952]) address review comments, refs #6546

[Thijs Triemstra]

(In [39954]) Merge replace-text-6546: Replace usage of twisted.python.text in twisted.lore.

Author: thijs Reviewer: rwall, tom.prince Fixes: #6546 Refs: #6341

Removes the usage of twisted.python.text in twisted.lore because it's being deprecated. Adds basic tests for lore.texi as well.