Opened 9 years ago
Last modified 8 years ago
#6341 enhancement assigned
Replace usage of twisted.python.text and deprecate it
Reported by: | Thijs Triemstra | Owned by: | Thijs Triemstra |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Thijs Triemstra | Branch: |
branches/py3-text-6341-2
branch-diff, diff-cov, branch-cov, buildbot |
Author: | thijs |
Description (last modified by )
Change History (25)
comment:1 Changed 9 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/py3-text-6341 |
comment:3 follow-up: 4 Changed 9 years ago by
Or uses of it could be replaced by textwrap
and twisted.python.text
could be deprecated and eventually removed.
comment:4 Changed 9 years ago by
Keywords: | review added |
---|
Replying to exarkun:
Or uses of it could be replaced by
textwrap
andtwisted.python.text
could be deprecated and eventually removed.
Sounds good. Let's port it (which was quite simple) and file new tickets to replace usage and deprecate it. I'd like to see it's supported on py3 until it's removed in x releases. Current coverage is 93%.
Forced a new build, please review.
comment:5 Changed 9 years ago by
Let's port it (which was quite simple) and file new tickets to replace usage and deprecate it
I appreciate that you've already done some of the porting work, but as a general plan, this doesn't sound great. Why do more work instead of less work?
comment:6 Changed 9 years ago by
Alright, if you put it that way. I thought there might've been something with value in there.. :)
- Couldn't find any usage of
stringyString
,isMultiline
,endsInNewline
, andremoveLeadingBlanks
, these can be deprecated directly. greedyWrap
is used once:./twisted/conch/insults/window.py:599: wrappedLines = tptext.greedyWrap(inputLines.pop(0), width)
and it's alias wordWrap
twice in twisted/python/usage.py
removeLeadingTrailingBlanks
is used quite a lot, but only, in lore..splitQuoted
is used in mail, python, words.. quite a alot.strFile
is used twice intwisted/mail/imap4.py
So completely deprecating the module isn't possible without doing the work listed above.
comment:7 follow-up: 10 Changed 9 years ago by
greedyWrap
is just textwrap.wrap
.
splitQuoted
is just shlex.split
.
At first glance, removeLeadingTrailingBlanks
looks really, *really* like str.strip
.
strFile
could be made into a private helper for imap4.py
.
comment:8 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Keywords: | review removed |
Milestone: | Python-3.x |
Owner: | set to Thijs Triemstra |
Status: | new → assigned |
Summary: | Port twisted.python.text to Python 3 → Replace usage of twisted.python.text and deprecate it |
Thanks for the review ;) Updating ticket title and description.
comment:9 Changed 9 years ago by
comment:10 Changed 9 years ago by
Replying to exarkun:
greedyWrap
is justtextwrap.wrap
.
Replaced.
splitQuoted
is justshlex.split
.
Replaced.
At first glance,
removeLeadingTrailingBlanks
looks really, *really* likestr.strip
.
It's used here:
./twisted/lore/tree.py: data = cStringIO.StringIO(text.removeLeadingTrailingBlanks(data)) ./twisted/lore/texi.py: self.writer(text.removeLeadingTrailingBlanks(buf.getvalue())) ./twisted/lore/slides.py: data = text.removeLeadingTrailingBlanks(data) ./twisted/lore/latex.py: self.writer(text.removeLeadingTrailingBlanks(buf.getvalue())) ./twisted/lore/latex.py: self.writer(text.removeLeadingTrailingBlanks('\n'.join(lines)))
Any suggestions for a better version are welcome but made it a private helper for lore as lore.tree.removeLeadingTrailingBlanks
.
strFile
could be made into a private helper forimap4.py
.
Added it as imap4._strFile
.
comment:11 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
Status: | assigned → new |
Added a deprecation warning in twisted.python
, similar to the deprecations in twisted.scripts
:
$ python -3 -c "from twisted.python import text" -c:1: DeprecationWarning: twisted.python.text was deprecated in Twisted 13.1.0: text has been deprecated.
Forced a new build, up for review.
comment:12 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
- The following doesn't emit a deprecation warning, but probably should.
#!/python from twisted.python.text import stringyString
I thinktwisted.protocols.telnet
is a better example. Note, this will involve suppressing the warning from the toplevel import, and perhaps more work to test the deprecation. (This probably also means the tests don't need to be touched) - There are some twistedchecker errors.
- Is there a good reason to port the module? (I haven't looked at these changes, except to note that we don't have full test coverage)
- There don't appear to be tests for the copied functions. (And the one in lore should probably be private.)
- The core topfile should be a
.removal
. - The lack of test coverage for almost all of the changes makes me sad. (Most of the changed code isn't exercised, let alone tested)
comment:15 Changed 9 years ago by
Replying to tom.prince:
- There are some twistedchecker errors.
Fixed.
- Is there a good reason to port the module? (I haven't looked at these changes, except to note that we don't have full test coverage)
I guess not, removed it from the ported list.
- There don't appear to be tests for the copied functions. (And the one in lore should probably be private.)
The new ones in t.lore.tree
are covered according to coverage. Copied the tests for imap4._strFile
.
- The core topfile should be a
.removal
.
Fixed.
comment:16 Changed 9 years ago by
Is this meant to be in review? In any case, here are a couple of comments:
- There appears to be some debugging changes or something in
twisted/conch/insults/window.py
(a use oft.p.text
). - The code in lore gets run, but it doesn't appear that its behavior isn't really tested. In any case, it's behavior is straightforward enough that it should be easy to write tests for it.
comment:17 Changed 9 years ago by
No, not yet (partly because of point 1 of your review), but thanks anyway.
comment:19 Changed 9 years ago by
Branch: | branches/py3-text-6341 → branches/py3-text-6341-2 |
---|
(In [38391]) Branching to 'py3-text-6341-2'
comment:20 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
twisted/lore/latex.py
:- Added test coverage for changed lines
twisted/lore/slides.py
:- Added test coverage for changed lines
twisted/lore/tree.py
:- Added test coverage for new methods (but
_removeLeadingBlanks
doesn't make sense to me, seems buggy?)
- Added test coverage for new methods (but
twisted/lore/texi.py
:- Added test coverage for changed lines in new
test_texi
case
- Added test coverage for changed lines in new
twisted/mail/imap4.py
:- Added test coverage for changed lines
twisted/python/usage.py
:- Changes were already covered (also see #1601)
twisted/words/protocols/irc.py
:- Added test coverage based on some of Wolf's code from #5329
twisted/conch/insults/window.py
:- Added test coverage
twisted.python.text
:- Changed the deprecation to mimic the telnet deprecation (added in r14536) and importing it in different ways displays the
DeprecationWarning
, only I'm not sure how to test/suppress this, any pointers are welcome.
- Changed the deprecation to mimic the telnet deprecation (added in r14536) and importing it in different ways displays the
Forced a new build, changes here: [log:branches/py3-text-6341-2].
comment:21 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:22 follow-up: 23 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
OK, it may be because I'm tired, or because the review queue is very long, but I think this ticket is improper. A 1500 lines diff is not a reasonable thing to do, in particular in this case. Please open at least 4 other tickets to handle changes per subprojects. Sorry for the delay in responding on this.
comment:23 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Status: | new → assigned |
Replying to therve:
OK, it may be because I'm tired, or because the review queue is very long, but I think this ticket is improper. A 1500 lines diff is not a reasonable thing to do, in particular in this case. Please open at least 4 other tickets to handle changes per subprojects. Sorry for the delay in responding on this.
Thanks for the review. I opened 4 additional tickets (see updated ticket description for ticket nrs) for the subprojects. I'll leave this ticket open as the main ticket that will actually deprecate the module and replace it's usage in core, once these other tickets have been resolved.
comment:24 Changed 9 years ago by
comment:25 Changed 8 years ago by
I made a port to Python 3 of twisted.python.usage
which used twisted.python.text.wordWrap
in #7038 and thus I replaced the usage of wordWrap
by textwrap.wrap
.
(In [37342]) Branching to 'py3-text-6341'