Opened 4 years ago

Last modified 3 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 Thijs Triemstra)

twisted.python.text can be deprecated after replacing usage of it.

This ticket covers the actual deprecation and replacement in core, and can be closed once these subtasks for the subprojects have been completed:

Change History (25)

comment:1 Changed 4 years ago by Thijs Triemstra

Author: thijs
Branch: branches/py3-text-6341

(In [37342]) Branching to 'py3-text-6341'

comment:2 Changed 4 years ago by Thijs Triemstra

(In [37344]) port t.p.text, add news file. refs #6341

comment:3 Changed 4 years ago by Jean-Paul Calderone

Or uses of it could be replaced by textwrap and twisted.python.text could be deprecated and eventually removed.

comment:4 in reply to:  3 Changed 4 years ago by Thijs Triemstra

Keywords: review added

Replying to exarkun:

Or uses of it could be replaced by textwrap and twisted.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 4 years ago by Jean-Paul Calderone

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 4 years ago by Thijs Triemstra

Alright, if you put it that way. I thought there might've been something with value in there.. :)

  1. Couldn't find any usage of stringyString, isMultiline, endsInNewline, and removeLeadingBlanks, these can be deprecated directly.
  2. 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

  1. removeLeadingTrailingBlanks is used quite a lot, but only, in lore..
  2. splitQuoted is used in mail, python, words.. quite a alot.
  3. strFile is used twice in twisted/mail/imap4.py

So completely deprecating the module isn't possible without doing the work listed above.

comment:7 Changed 4 years ago by Jean-Paul Calderone

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 4 years ago by Thijs Triemstra

Description: modified (diff)
Keywords: review removed
Milestone: Python-3.x
Owner: set to Thijs Triemstra
Status: newassigned
Summary: Port twisted.python.text to Python 3Replace usage of twisted.python.text and deprecate it

Thanks for the review ;) Updating ticket title and description.

comment:9 Changed 4 years ago by Thijs Triemstra

(In [37349]) replace usage of t.python.text, add news files. refs #6341

comment:10 in reply to:  7 Changed 4 years ago by Thijs Triemstra

Replying to exarkun:

greedyWrap is just textwrap.wrap.

Replaced.

splitQuoted is just shlex.split.

Replaced.

At first glance, removeLeadingTrailingBlanks looks really, *really* like str.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 for imap4.py.

Added it as imap4._strFile.

comment:11 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

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 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. The following doesn't emit a deprecation warning, but probably should.
    #!/python
    from twisted.python.text import stringyString
    
    I think twisted.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)
  2. There are some twistedchecker errors.
  3. 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)
  4. There don't appear to be tests for the copied functions. (And the one in lore should probably be private.)
  5. The core topfile should be a .removal.
  6. 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:13 Changed 4 years ago by Thijs Triemstra

(In [38002]) address review comments, refs #6341

comment:14 Changed 4 years ago by Thijs Triemstra

(In [38041]) address review comments, refs #6341

comment:15 Changed 4 years ago by Thijs Triemstra

Replying to tom.prince:

  1. There are some twistedchecker errors.

Fixed.

  1. 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.

  1. 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.

  1. The core topfile should be a .removal.

Fixed.

comment:16 Changed 4 years ago by Tom Prince

Is this meant to be in review? In any case, here are a couple of comments:

  1. There appears to be some debugging changes or something in twisted/conch/insults/window.py (a use of t.p.text).
  2. 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 4 years ago by Thijs Triemstra

No, not yet (partly because of point 1 of your review), but thanks anyway.

comment:18 Changed 4 years ago by Richard Wall

See also #1601 where one use of text.wordWrap is being removed.

comment:19 Changed 4 years ago by Thijs Triemstra

Branch: branches/py3-text-6341branches/py3-text-6341-2

(In [38391]) Branching to 'py3-text-6341-2'

comment:20 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
  1. twisted/lore/latex.py:
    1. Added test coverage for changed lines
  2. twisted/lore/slides.py:
    1. Added test coverage for changed lines
  3. twisted/lore/tree.py:
    1. Added test coverage for new methods (but _removeLeadingBlanks doesn't make sense to me, seems buggy?)
  4. twisted/lore/texi.py:
    1. Added test coverage for changed lines in new test_texi case
  5. twisted/mail/imap4.py:
    1. Added test coverage for changed lines
  6. twisted/python/usage.py:
    1. Changes were already covered (also see #1601)
  7. twisted/words/protocols/irc.py:
    1. Added test coverage based on some of Wolf's code from #5329
  8. twisted/conch/insults/window.py:
    1. Added test coverage
  9. twisted.python.text:
    1. 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.

Forced a new build, changes here: [log:branches/py3-text-6341-2].

comment:21 Changed 4 years ago by Thijs Triemstra

Description: modified (diff)

comment:22 Changed 4 years ago by therve

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 in reply to:  22 Changed 4 years ago by Thijs Triemstra

Description: modified (diff)
Status: newassigned

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 4 years ago by 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.

comment:25 Changed 3 years ago by Jonathan Ballet

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.

Note: See TracTickets for help on using tickets.