Opened 9 years ago

Last modified 8 years ago

#6341 enhancement new

— at Replace usage of twisted.python.text and deprecate itVersion 21

Reported by: Thijs Triemstra Owned by:
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.

Change History (21)

comment:1 Changed 9 years ago by Thijs Triemstra

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

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

comment:2 Changed 9 years ago by Thijs Triemstra

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

comment:3 Changed 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 years ago by Thijs Triemstra

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

comment:14 Changed 9 years ago by Thijs Triemstra

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

comment:15 Changed 9 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 9 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 9 years ago by Thijs Triemstra

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

comment:18 Changed 9 years ago by Richard Wall

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

comment:19 Changed 8 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 8 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 8 years ago by Thijs Triemstra

Description: modified (diff)
Note: See TracTickets for help on using tickets.