Opened 11 years ago

Last modified 4 years ago

#1601 defect new

t.py.text.greedyWrap makes t.py.usage.Options.longdesc ugly

Reported by: David Reid Owned by: Stephen Solis
Priority: low Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra Branch: branches/ugly-usage-wrapping-1601
branch-diff, diff-cov, branch-cov, buildbot
Author: stephsolis

Description

The lack of a word wrap implementation other than greedyWrap makes it difficult to convey useful information in usage.Options.longdesc. Specifically it eats single newlines and indentation whitespace. I think a more ideal alternative implementation would:

  1. only wraps lines that are longer than the specified width
  2. only eat new lines in the absence of beginning whitespace
  3. support two spaces at the end of sentences

Attachments (7)

1601.patch (536 bytes) - added by Tom Davis 6 years ago.
Removed wordWrap() from Options processing
1601v2.patch (1.6 KB) - added by Stephen Solis 4 years ago.
Added unit test and news file
1601v3.patch (2.3 KB) - added by Stephen Solis 4 years ago.
Applied review suggestions.
1601v4.patch (1.3 KB) - added by Stephen Solis 4 years ago.
Applied more review suggestions
1601v5.patch (5.0 KB) - added by Stephen Solis 4 years ago.
Added a better word wrap implementation and more unit tests
1601v6.patch (7.9 KB) - added by Stephen Solis 4 years ago.
Addressed more issues from review
1601v7.patch (8.5 KB) - added by Stephen Solis 4 years ago.
Addressed more issues from review

Download all attachments as: .zip

Change History (32)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Keywords: easy added
Type: enhancementdefect

I suggest eliminating the automatic wrapping. Most terminals are reasonably sized most of the time. Reasonable width long descs will look okay. Perhaps later on we can come up with a more advanced text typesetting system. ;)

Changed 6 years ago by Tom Davis

Attachment: 1601.patch added

Removed wordWrap() from Options processing

comment:2 Changed 6 years ago by Tom Davis

Keywords: review added
Owner: Glyph deleted

I believe this covers the requirement laid out by exarkun. There was no test to ensure longdesc was actually wrapped in the first place (at least that I could find), so this patch does not include one to prove the opposite.

comment:3 in reply to:  2 Changed 6 years ago by therve

Keywords: review removed
Owner: set to Tom Davis

Replying to binjured:

There was no test to ensure longdesc was actually wrapped in the first place (at least that I could find), so this patch does not include one to prove the opposite.

Good observation, but wrong conclusion: please add a test to the changed code (it shouldn't be really hard). Also, please add NEWS fragment. Thanks.

comment:4 Changed 4 years ago by Itamar Turner-Trauring

Owner: Tom Davis deleted

Re-assign to nobody so it's clearer to GSoC candidates they can work on this ticket.

comment:5 Changed 4 years ago by Stephen Solis

Owner: set to Stephen Solis
Status: newassigned

Changed 4 years ago by Stephen Solis

Attachment: 1601v2.patch added

Added unit test and news file

comment:6 Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted
Status: assignednew

comment:7 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Reviewing...

comment:8 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/ugly-usage-wrapping-1601

(In [38131]) Branching to 'ugly-usage-wrapping-1601'

comment:9 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Stephen Solis
Status: assignednew

Code Review

Thanks stephsolis for continuing the work on this ticket.

Notes:

  • Build Failures (see below)
  • Patch applies cleanly to trunk
  • New tests pass with Python2.7 on my Fedora 18 x86_64 laptop
  • I compared the usage output of one the twisted.names gethostbyname example and it does look much neater. Indented lines remain indented and bulleted lists remain bulleted.
[richard@zorin examples]$ chbranch Twisted trunk
[richard@zorin examples]$ python gethostbyname.py
Usage: gethostbyname.py HOSTNAME
Options:
      --version  Display Twisted version and exit.
      --help     Display this help and exit.

Print the IP address for a given hostname. eg

python gethostbyname.py www.google.com

This script does a host lookup using the default Twisted Names resolver, a
chained resolver, which attempts to lookup a name from: * local hosts file *
memory cache of previous lookup results * system recursive DNS servers

ERROR: Wrong number of arguments.
[richard@zorin examples]$ chbranch Twisted ugly-usage-wrapping-1601
[richard@zorin examples]$ python gethostbyname.py
Usage: gethostbyname.py HOSTNAME
Options:
      --version  Display Twisted version and exit.
      --help     Display this help and exit.

Print the IP address for a given hostname. eg

 python gethostbyname.py www.google.com

This script does a host lookup using the default Twisted Names
resolver, a chained resolver, which attempts to lookup a name from:
 * local hosts file
 * memory cache of previous lookup results
 * system recursive DNS servers

ERROR: Wrong number of arguments.

Please address or answer the following points:

  1. twisted/test/test_usage.py
    1. The patch contains a new blank line at the top of this file - please remove it.
    2. Remove all trailing whitespace.
    3. The very long line in the test options breaks the coding standard. Use parenthesis to keep it < 80 columns.
    4. Add epytext link markup to the new test docstrings. eg L{usage.Options.getUsage}.
    5. assertGreater was introduced in Python 2.7 so the test fails on Python2.6 buildslaves. Use assertEquals instead. (https://buildbot.twistedmatrix.com/builders/debian-easy-no-zope-py2.6-epoll/builds/451/steps/epoll/logs/problems)
    6. Remove "Check that" from your new test docstring (see #6301 for test docstring guidelines.)
    7. Consider adding your new test to the existing HelpStringTest case which already contains a related test for whitespace stripping in options.
  2. twisted/python/usage.py
    1. The parenthesis are no longer necessary when assigning to longdesc.
      500                          longdesc = ('\n' +
      501                                      '\n'.join(text.wordWrap(longdesc, width)).strip()
      502                                      + '\n')
       500                 longdesc = ('\n' + longdesc.strip() + '\n')
      

Please address or answer the numbered comments above and submit a further patch (against source:branches/ugly-usage-wrapping-1601) for review.

Thanks again.

-RichardW.

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

assertGreater was introduced in Python 2.7 so the test fails on Python2.6 buildslaves. Use assertEquals instead

Use assertEqual. assertEquals is deprecated, or about to be. Also, the easiest transformation is probably actually to use assertTrue - ie, assertTrue(len(opt.longdesc.splitlines()[2]) > 80).

comment:11 in reply to:  9 ; Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted

Thank you very much for the review!

Replying to rwall:

  1. twisted/test/test_usage.py
    1. The patch contains a new blank line at the top of this file - please remove it.

Done.

  1. Remove all trailing whitespace.

I couldn't see any, is it still there?

  1. The very long line in the test options breaks the coding standard. Use parenthesis to keep it < 80 columns.

Done.

  1. Add epytext link markup to the new test docstrings. eg L{usage.Options.getUsage}.

Do I need this even in test_longdescNotWrapped or only in GetUsageTestCase (which I removed)?

  1. assertGreater was introduced in Python 2.7 so the test fails on Python2.6 buildslaves. Use assertEquals instead. (https://buildbot.twistedmatrix.com/builders/debian-easy-no-zope-py2.6-epoll/builds/451/steps/epoll/logs/problems)

OK.

  1. Remove "Check that" from your new test docstring (see #6301 for test docstring guidelines.)

Got it.

  1. Consider adding your new test to the existing HelpStringTest case which already contains a related test for whitespace stripping in options.

Did that.

  1. twisted/python/usage.py
    1. The parenthesis are no longer necessary when assigning to longdesc.

Done.

Changed 4 years ago by Stephen Solis

Attachment: 1601v3.patch added

Applied review suggestions.

comment:12 Changed 4 years ago by Richard Wall

(In [38154]) Apply 1601v3.patch refs #1601

comment:13 in reply to:  11 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to Stephen Solis

Replying to stephsolis:

  1. Remove all trailing whitespace.

I couldn't see any, is it still there?

It was deleted when you rewrapped the longdesc in the test module.

I removed some other trailing whitespace from the end of usage.py though.

  1. Add epytext link markup to the new test docstrings. eg L{usage.Options.getUsage}.

Do I need this even in test_longdescNotWrapped or only in GetUsageTestCase (which I removed)?

Yes, you should really use L{} when you refer to any public twisted or stdlib object. See source:trunk/twisted/python/test/test_constants.py for some examples of recently written tests which adhere to current best practice.

  1. Consider adding your new test to the existing HelpStringTest case which already contains a related test for whitespace stripping in options.

Did that.

I personally think you could get rid of the new GetUsageOptions class in test_usage. Why not just assign to self.nice.longdesc in test_longdescNotWrapped or add your new longdesc to WellBehaved?

Anyway, I applied your latest patch to the branch and started a build. See:

There's one new twistedchecker error which you need to fix (possibly by removing GetUsageOptions)

Then please submit another patch for review.

Thanks.

-RichardW.

Changed 4 years ago by Stephen Solis

Attachment: 1601v4.patch added

Applied more review suggestions

comment:14 Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted

I made the patch against the latest branches/ugly-usage-wrapping-1601. Is this the correct thing to do, or should I have made it against trunk or something else?

comment:15 Changed 4 years ago by Richard Wall

(In [38159]) apply 1601v4.patch from stephsolis. refs #1601

comment:16 in reply to:  14 ; Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to Stephen Solis

Replying to stephsolis:

I made the patch against the latest branches/ugly-usage-wrapping-1601. Is this the correct thing to do, or should I have made it against trunk or something else?

Thanks stephsolis.

You did the right thing. I applied the v4 patch to the branch and started another build

There are problems though.

  1. I only just noticed that you're testing the length of the Options.longdesc. But the longdesc attribute isn't changed in place. You should test the output of Options.getUsage.
  1. I'd also like to know what dreid or exarkun think about this change.
  1. Should this branch also fix the wrapping of existing commands that have long longdescs - which now look quite ugly on narrow consoles? eg
    [richard@zorin ugly-usage-wrapping-1601]$ twistd conch --help
    Usage: twistd [options] conch [-i <interface>] [-p <port>] [-d <dir>]
    Options:
      -i, --interface=       local interface to which we listen [default: ]
      -p, --port=            Port on which to listen [default: tcp:22]
      -d, --data=            directory to look for host keys in [default: /etc]
          --moduli=          directory to look for moduli in (if different from
                             --data)
          --help-auth-type=  Show help for a particular authentication type.
          --version          Display Twisted version and exit.
          --help             Display this help and exit.
          --auth=            Specify an authentication method for the server.
          --help-auth        Show all authentication methods available.
    
    Makes a Conch SSH server.  If no authentication methods are specified, the default authentication methods are UNIX passwords, SSH public keys, and PAM if it is available.  If --auth options are passed, only the measures specified will be used.
    
  1. Would it be ok to introduce a new "formatLongdesc" method which (for now) simply strips the longdesc and which can be easily tested?
  1. How many tests should there be for this change? Should we eg test that there is no change in indentation and no wrapping, even when there are spaces and hyphens in the long line?

-RichardW.

comment:17 in reply to:  16 ; Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted

Thanks for the suggestions.

I personally think removing automatic line wrapping altogether may not be such a good idea because we can't always assume longdesc will have a reasonable width (as you showed with conch).

The Python standard library already has text wrapping functionality in the textwrap module, so do you know why another (somewhat hacky IMHO) text wrapper was implemented in text.greedyWrap? textwrap has been there since Python 2.3, so I propose getting rid of greedyWrap and using textwrap.wrap instead. I added a new word wrap implementation based on textwrap in my latest patch, but should a new bug be opened for this? Should greedyWrap be deprecated? I'm not sure where else greedyWrap is used, but why does it try to combine lines into one (ie. "foo\nbar\ndaz" yields "foo bar daz" instead of "foo\nbar\ndaz")? I added a unit test for that functionality (I assume it is intentional?), but I don't think that's what we want to do to longdesc, is it?

Anyway, here's the output on the gethostbyname example...

Usage: gethostbyname.py HOSTNAME
Options:
      --version  Display Twisted version and exit.
      --help     Display this help and exit.

Print the IP address for a given hostname. eg

 python gethostbyname.py www.google.com

This script does a host lookup using the default Twisted Names
resolver, a chained resolver, which attempts to lookup a name from:
 * local hosts file
 * memory cache of previous lookup results
 * system recursive DNS servers

ERROR: Wrong number of arguments.

...and with conch

Usage: twistd [options] conch [-i <interface>] [-p <port>] [-d <dir>]
Options:
  -i, --interface=       local interface to which we listen [default: ]
  -p, --port=            Port on which to listen [default: tcp:22]
  -d, --data=            directory to look for host keys in [default: /etc]
      --moduli=          directory to look for moduli in (if different from
                         --data)
      --help-auth-type=  Show help for a particular authentication type.
      --version          Display Twisted version and exit.
      --help             Display this help and exit.
      --auth=            Specify an authentication method for the server.
      --help-auth        Show all authentication methods available.

Makes a Conch SSH server.  If no authentication methods are specified, the
default authentication methods are UNIX passwords, SSH public keys, and PAM if
it is available.  If --auth options are passed, only the measures specified will
be used.

My unit tests now check that lines _are_ wrapped and that indentation is not changed after wrapping lines. I think this way better fits the criteria given in the bug description.

Changed 4 years ago by Stephen Solis

Attachment: 1601v5.patch added

Added a better word wrap implementation and more unit tests

comment:18 in reply to:  17 ; Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to Stephen Solis

Replying to stephsolis:

I personally think removing automatic line wrapping altogether may not be such a good idea because we can't always assume longdesc will have a reasonable width (as you showed with conch).

I think exarkun's point was that wrapping the structured text in command help is hard to get right.

Your latest approach - wrapping each line - doesn't work when you attempt to wrap the lines of a paragraph which has already been wrapped, as in gethostbyname. The line breaks appear in the wrong place. eg

In [28]: o = Options()

In [29]: o.longdesc = """
   ....: Print the IP address for a given hostname. eg
   ....:  python gethostbyname.py www.google.com
   ....: This script does a host lookup using the default Twisted Names
   ....: resolver, a chained resolver, which attempts to lookup a name from:
   ....:          * local hosts file
   ....:          * memory cache of previous lookup results
   ....:          * system recursive DNS servers
   ....: """
   ....:

In [30]: print o.getUsage(width=50)
Options:
      --version  Display Twisted version and exit.
      --help     Display this help and exit.

Print the IP address for a given hostname. eg
 python gethostbyname.py www.google.com
This script does a host lookup using the default
Twisted Names
resolver, a chained resolver, which attempts to
lookup a name from:
         * local hosts file
         * memory cache of previous lookup results
         * system recursive DNS servers

So is worth trying to dynamically wrap command line help based on the detected console width? Probably not.

Other common commands don't attempt it. Try rm --help in a <80 columns terminal.

It's better to preformat the paragraphs of the help text to a lowest common terminal width.

If you're not convinced, I suggest you join irc #twisted-dev and ask for some feedback / guidance from exarkun or dreid.

The Python standard library already has text wrapping functionality in the textwrap module, so do you know why another (somewhat hacky IMHO) text wrapper was implemented in text.greedyWrap? textwrap has been there since Python 2.3,

Ah, but this code predates Python 2.3.

  • log:trunk/twisted/python/text.py
  • log:trunk/twisted/python/usage.py

so I propose getting rid of greedyWrap and using textwrap.wrap instead. I added a new word wrap implementation based on textwrap in my latest patch, but should a new bug be opened for this? Should greedyWrap be deprecated? I'm not sure where else greedyWrap is used,

Actually I just noticed that thijs is already working on this so we should probably coordinate with him. See:

  • #6341
  • log:branches/py3-text-6341

My unit tests now check that lines _are_ wrapped and that indentation is not changed after wrapping lines. I think this way better fits the criteria given in the bug description.

I don't like the reliance on line offsets in the new tests. It seems fragile. What happens if another default option is added or removed.

Here's what I think you should do:

  1. Remove all longdesc text wrapping (as in the original patches 1-4)
  1. Add Options._getLongdesc: Refactor all the discovery and formatting (leading and trailing linebreak only) of longDesc into this new private method.

This simplifies the getUsage method, simplifies testing of longdesc formatting and also allows you to provide tests and coverage for the case where main.doc is used as longdesc.

https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r38159/twisted_python_usage.html#n531

  1. Add the following new tests
    1. HelpStringTest.test_getLongdescFromAttribute
    2. HelpStringTest.test_getLongdescFromMainDoc

I wouldn't bother testing for specific line lengths or for indentation. Just test that the assigned longdesc is somewhere (unmodified) in the output of _getLongdesc.

Use TestCase.patch to temporarily modify main.doc.

https://twistedmatrix.com/documents/current/api/twisted.trial.unittest.SynchronousTestCase.html#patch

  1. Add full epytext docstring to Options.getUsage
  1. Find and manually wrap any overly long longdesc. I don't think there are many, and in cases where main.doc is used, these should already be wrapped at 80 columns to meet the coding standard.

I think this will be a nice overall improvement to usage.Options - better tested, better documented, shorter / less complex getUsage function, and most importantly, neater command line help.

Thanks for your perseverance; hopefully this ticket won't drag on much longer.

-RichardW.

[richard@zorin ugly-usage-wrapping-1601]$ ack longdesc

twisted/scripts/trial.py
104:    longdesc = ("trial loads and executes a suite of unit tests, obtained "

twisted/test/test_usage.py
27:    longdesc = ("\nA test documentation string.\n"
395:        wrapping lines in longdesc.

twisted/application/app.py
542:    longdesc = ("twistd reads a twisted.application.service.Application out "

twisted/tap/telnet.py
16:    longdesc = "Makes a telnet server to a Python shell."

twisted/tap/ftp.py
34:    longdesc = ''

twisted/tap/socks.py
26:    longdesc = "Makes a SOCKSv4 server."

twisted/tap/portforward.py
14:    longdesc = 'Port Forwarder.'

twisted/mail/tap.py
69:    longdesc = """

twisted/web/tap.py
47:    longdesc = """\

twisted/lore/scripts/lore.py
27:    longdesc = "lore converts documentation formats."

twisted/lore/xhtml1-strict.dtd
641:   description using the alt and longdesc attributes.
652:  longdesc    %URI;          #IMPLIED

twisted/lore/xhtml1-transitional.dtd
391:  longdesc    %URI;          #IMPLIED
829:   description using the alt and longdesc attributes.
839:  longdesc    %URI;          #IMPLIED

twisted/runner/procmontap.py
37:    longdesc = """\

twisted/conch/scripts/conch.py
24:    longdesc = ("conch is a SSHv2 client that allows logging into a remote "

twisted/conch/scripts/ckeygen.py
27:    longdesc = "ckeygen manipulates public/private keys in various ways."

twisted/conch/scripts/cftp.py
25:    longdesc = ("cftp is a client for logging into a remote machine and "

twisted/conch/tap.py
24:    longdesc = ("Makes a Conch SSH server.  If no authentication methods are "

twisted/python/usage.py
528:        if not (getattr(self, "longdesc", None) is None):
529:            longdesc = self.longdesc
533:                longdesc = __main__.__doc__
535:                longdesc = ''
537:        if longdesc:
539:            for line in longdesc.strip().splitlines():

doc/historic/2003/europython/twisted.html
322:    longdesc = 'Finger Server'

comment:19 in reply to:  18 Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted

Replying to rwall:

So is worth trying to dynamically wrap command line help based on the detected console width? Probably not.

Other common commands don't attempt it. Try rm --help in a <80 columns terminal.

It's better to preformat the paragraphs of the help text to a lowest common terminal width.

Yes, on second thought you are absolutely right: trying to think of every possibility for automatically wrapping stuff in longdesc would take a lot of effort and probably make getUsage needlessly complex.

Actually I just noticed that thijs is already working on this so we should probably coordinate with him. See:

  • #6341
  • log:branches/py3-text-6341

I'll be sure to keep an eye on #6341. I'm happy to see we're getting rid of t.p.text - there's no sense in recreating functionality already in the standard library!

Here's what I think you should do:

I think I've addressed all your concerns (ack is incredibly useful). I also added a test_whitespaceStripLongdesc like test_whitespaceStripFlagsAndParameters.

Thanks for your perseverance; hopefully this ticket won't drag on much longer.

I just want to take a second to say thank you, rwall, for helping a first-time Twisted contributor like me learn the ropes. It's great to be part of a project that's simultaneously really cool and powerful, and also has developers as patient and thorough as yourself. Again, thanks!

Changed 4 years ago by Stephen Solis

Attachment: 1601v6.patch added

Addressed more issues from review

comment:20 Changed 4 years ago by Thijs Triemstra

Author: rwallstephsolis
Keywords: review removed
Owner: set to Stephen Solis

Thanks for your patch. Looks like this ticket is almost there so here are some coding standard comments:

  1. in twisted/test/test_usage.py:
    1. 2 blank lines between tests you change or add (eg. line 388)
    2. 3 between classes (eg. line 407)
  2. in twisted/python/usage.py:
    1. missing blank lines (see 1.1 and 1.2)
    2. the docstring contains incorrect epytext for the listing. See 2.2. Lists section here.
    3. place 2 of them at the end, like:
      It includes descriptions of the following in this order, if they exist::
      

comment:21 Changed 4 years ago by Thijs Triemstra

  1. oh and width is missing @param and @type epytext
    def getUsage(self, width=None):
    

Changed 4 years ago by Stephen Solis

Attachment: 1601v7.patch added

Addressed more issues from review

comment:22 in reply to:  20 Changed 4 years ago by Stephen Solis

Keywords: review added
Owner: Stephen Solis deleted

Replying to thijs:

  1. the docstring contains incorrect epytext for the listing.

I assume you meant an ordered list is more appropriate?

  1. place 2 of them at the end, like:

Why do you want the list in a literal block?

comment:23 Changed 4 years ago by Tom Prince

(In [38710]) Apply 1601v7.patch from stephsolis.

Refs: #1601

comment:24 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Stephen Solis
  1. test_whitespaceStripLongdesc: The docstring says what is supposed to happen, but not what is to cause it to happen. Either say when whitespace is stripped or (probably better) reword to say what is doing the stripping "L{..._getLongdesc} strips ....".
  2. Taken together, the docstrings for test_getLongdescFromAttribute and test_getLongdescFromMainDoc suggest that it reads from both locations.
    • Please add some words to indicate when each behavior should happen.
    • Also, there should be tests for when both and when neither attribute is present.
  3. Options.getUsage and Options._getLongdesc are missing @return and @rtype epytext. (reported by twistedchecker)
  4. This should probably get a real news fragment. This is changing behavior in a user visible way. People may have been expecting longdesc to be wrapped, so we should tell them that they need to change their code.

comment:25 Changed 4 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Note: See TracTickets for help on using tickets.