Opened 4 years ago

Last modified 4 years ago

#6543 task new

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

Reported by: Thijs Triemstra Owned by: lvh
Priority: low Milestone:
Component: conch Keywords:
Cc: Thijs Triemstra, z3p Branch: branches/replace-text-conch-6543-2
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.conch.

Change History (13)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 4 years ago by Thijs Triemstra

Author: thijs
Branch: branches/replace-text-conch-6543

(In [38638]) Branching to 'replace-text-conch-6543'

comment:3 Changed 4 years ago by Thijs Triemstra

(In [38639]) replace usage of t.python.text, refs #6543

comment:4 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replaced the usage in twisted.conch and added tests for the uncovered lines.

Forced a build.

comment:5 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra

Thanks for working on this. A couple of things:

  1. DummyTerminal: Is there any reason it doesn't just unconditionally set self.lines to [] in __init__?
  2. TextOutputTests:
    1. test_focusReceived: use assertRaises
    2. test_render: There should be a test for padding, and for truncation.
  3. TextOutputAreaTests
    1. The tests don't capture the position that things get written at. Record tuples of position and text written?
    2. There should be some multiline tests. In particular, in the wrapping case, it should test it displays the right number of physical lines, as opposed to logical lines.
    3. I wonder how come TextOutputArea doesn't pad, where as TextOutput does. This should perhaps be a seperate ticket.
  4. Neither widget handles the 0 height case (again, this should be a seperate ticket)

Please resubmit for review after addressing the above points.

comment:6 Changed 4 years ago by Thijs Triemstra

(In [38984]) address review comments, refs #6543

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

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for your review.

Replying to tom.prince:

Thanks for working on this. A couple of things:

  1. DummyTerminal: Is there any reason it doesn't just unconditionally set self.lines to [] in __init__?

Fixed.

  1. TextOutputTests:
    1. test_focusReceived: use assertRaises

Fixed.

  1. test_render: There should be a test for padding, and for truncation.

Added 2 tests.

  1. TextOutputAreaTests
    1. The tests don't capture the position that things get written at. Record tuples of position and text written?

Fixed.

  1. There should be some multiline tests. In particular, in the wrapping case, it should test it displays the right number of physical lines, as opposed to logical lines.

Fixed..

comment:8 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. There should be some multiline tests. In particular, in the wrapping case, it should test it displays the right number of physical lines, as opposed to logical lines.

Fixed..

The string used for testing still only has a single line. In particular, an input like

this is a
long
long string

When output to a 4x4 width widget should give

this
is a
long
long

I haven't test this, but it seems cutting off a wrapped line in the middle of the lines seems like a likely error. Also, what is the behavior if a single word is wider than the widget?

comment:9 Changed 4 years ago by Thijs Triemstra

Branch: branches/replace-text-conch-6543branches/replace-text-conch-6543-2

(In [40073]) Branching to 'replace-text-conch-6543-2'

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

Keywords: review added
Owner: Thijs Triemstra deleted

Replying to tom.prince:

The string used for testing still only has a single line. In particular, an input like

this is a
long
long string

When output to a 4x4 width widget should give

this
is a
long
long

Added a multiline test.

I haven't test this, but it seems cutting off a wrapped line in the middle of the lines seems like a likely error. Also, what is the behavior if a single word is wider than the widget?

Good point. Added a test that broke with the new textwrap.wrap but succeeded with the old greedyWrap. There's a comment in t.python.text.greedyWrap about single words:

# This single word is too long, it will be the whole line.
pass

So single words should never be broken into multiple lines according to the old implementation. Fixed this by using TextWrapper.break_long_words = False instead. Tests now both pass using the old greedyWrap and the new implementation, with full coverage for both classes as well.

Forced a build.

comment:11 Changed 4 years ago by lvh

Keywords: review removed
Owner: set to lvh

comment:12 Changed 4 years ago by lvh

Keywords: review added

There was a twistedchecker failure:

W9207: 57,0:DummyTerminal: Missing a blank line before epytext markups

It doesn't appear to actually be the case. I've re-ran the build to see if it's transient...

comment:13 Changed 4 years ago by lvh

Keywords: review removed

Oops, used a stale tab. Didn't intend to submit that back for review.

Note: See TracTickets for help on using tickets.