Opened 7 years ago
Closed 6 years ago
#6546 task closed fixed (fixed)
Replace usage of twisted.python.text in twisted.lore
Reported by: | Thijs Triemstra | Owned by: | Thijs Triemstra |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | lore | Keywords: | |
Cc: | Thijs Triemstra | Branch: |
branches/replace-text-6546
branch-diff, diff-cov, branch-cov, buildbot |
Author: | thijs |
Description
twisted.python.text is being deprecated (#6341) and its usage should be replaced in twisted.lore.
Change History (14)
comment:1 Changed 7 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/replace-text-6546 |
comment:3 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
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.
comment:4 follow-up: 6 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
Code Review
Hi thijs,
- branches/replace-text-6546/twisted/lore/slides.py
- 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.)
- Add a docstring for the "visitNode" function.
- branches/replace-text-6546/twisted/lore/test/test_lore.py
- 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?
- And "removeLeadingTrailingBlanks" only seems to add a trailing new line to the input without stripping any whitespace. Same goes for t.p.text.removeLeadingTrailingBlanks.
- twisted/lore/test/test_texi.py
- The variable name "pre" in "test_title" is confusing.
- branches/replace-text-6546/twisted/lore/test/test_latex.py
- 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?
- branches/replace-text-6546/twisted/lore/test/test_slides.py
- 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)
- Minor nit - I don't like the way the long import line has been
wrapped. I'd prefer:
- branches/replace-text-6546/twisted/lore/tree.py
- 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)
- I think you should just remove the calls to those two functions and add a blank line wherever "removeLeadingTrailingBlanks" was used.
- As far as I can see the two text functions never did anything
useful. The original (unstripped) line is returned.
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.
comment:6 follow-up: 7 Changed 6 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Thijs Triemstra to Richard Wall |
Thanks for your review Richard.
Replying to rwall:
- branches/replace-text-6546/twisted/lore/slides.py
- 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.
- 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?
- branches/replace-text-6546/twisted/lore/test/test_lore.py
- 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.
- And "removeLeadingTrailingBlanks" only seems to add a trailing new line to the input without stripping any whitespace. Same goes for t.p.text.removeLeadingTrailingBlanks.
- twisted/lore/test/test_texi.py
- The variable name "pre" in "test_title" is confusing.
Renamed.
- branches/replace-text-6546/twisted/lore/test/test_latex.py
- 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.
- branches/replace-text-6546/twisted/lore/test/test_slides.py
- 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.
comment:7 Changed 6 years ago by
Owner: | Richard Wall deleted |
---|
Replying to thijs:
lore is the only user of
t.p.text.removeLeadingBlanks
andremoveLeadingTrailingBlanks
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:
-RichardW.
comment:8 follow-ups: 9 10 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
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.
- 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.
- 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) - Similarily, the test for
MagicPointOutput.visitNode_body
doesn't test any of the interesting behavior. (And the one forvisit_pre
doesn't handle the case of multiple lines. - The same is true for
TexiSplitter.visit_title
. - 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.
comment:9 Changed 6 years ago by
Replying to tom.prince:
- 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.
comment:10 Changed 6 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
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..
- 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.
- 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.
- Similarily, the test for
MagicPointOutput.visitNode_body
doesn't test any of the interesting behavior. (And the one forvisit_pre
doesn't handle the case of multiple lines.
Expanded the test.
- The same is true for
TexiSplitter.visit_title
.
Expanded the test.
- 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.
comment:11 Changed 6 years ago by
Owner: | set to Richard Wall |
---|---|
Status: | new → assigned |
comment:12 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Thijs Triemstra |
Status: | assigned → new |
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:
- Build Failure
- Python3 -- I think that failure is unrelated and has now been fixed in trunk so we can ignore it.
- 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)
- source:branches/replace-text-6546/twisted/lore/slides.py
- C{xml.dom.minidom.Element} >> L{}
- source:branches/replace-text-6546/twisted/lore/latex.py
- C{xml.dom.minidom.Element} >> L{}
- source:branches/replace-text-6546/twisted/lore/tree.py
- @type lines: L{str} >> L{list} (I think)
- source:branches/replace-text-6546/twisted/lore/texi.py
- C{xml.dom.minidom.Element} >> L{}
Please merge after addressing the points above and checking for clean build results.
-RichardW.
comment:14 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [38632]) Branching to 'replace-text-6546'