#6393 enhancement closed fixed (fixed)

Utility function to pretty-format list of things

Reported by: cyli Owned by: tom.prince
Priority: low Milestone:
Component: trial Keywords: easy
Cc: francis.sylvain@… Branch: branches/listToPhrase-6393
(diff, github, buildbot, log)
Author: gen3, Chewy, jamesbroadhead Launchpad Bug:

Description

As per tom.prince's review in http://twistedmatrix.com/trac/ticket/6380, a utility function somewhere in twisted.python to produce strings like "ex1, ex2, or ex3" would be useful.

So pretty_format([3, 4, 5, 6, 7], 'and') would be '3, 4, 5, 6, and 7' or something similar.

Attachments (8)

pretty_print_6393.patch (593 bytes) - added by gen3 16 months ago.
the pretty print function in utils.py
6393.patch (2.0 KB) - added by Chewy 16 months ago.
6393.2.patch (2.6 KB) - added by Chewy 15 months ago.
6393.3.patch (4.7 KB) - added by Chewy 15 months ago.
6393.4.patch (5.3 KB) - added by Chewy 15 months ago.
6393.5.patch (5.1 KB) - added by Chewy 14 months ago.
6393.6.patch (5.6 KB) - added by jamesbroadhead 13 months ago.
6393.branch.patch (1.8 KB) - added by Chewy 13 months ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 16 months ago by cyli

  • Keywords easy added

comment:2 Changed 16 months ago by exarkun

Some code in Imaginary might help with this.

comment:3 Changed 16 months ago by Chewy

  • Cc francis.sylvain@… added
  • Owner set to Chewy
  • Status changed from new to assigned

comment:4 Changed 16 months ago by gen3

  • Owner changed from Chewy to gen3
  • Status changed from assigned to new

Changed 16 months ago by gen3

the pretty print function in utils.py

comment:5 Changed 16 months ago by gen3

  • Resolution set to fixed
  • Status changed from new to closed

this is my first patch.. will that do?
i dont know how i can edit the documentation of API reference. please help me. thank you :)

comment:6 follow-up: Changed 16 months ago by exarkun

  • Keywords review added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Hello. Thanks for your contribution. Please read ReviewProcess to learn about the workflow we use for the issue tracker.

comment:7 Changed 16 months ago by gen3

what should i add to my patch in order for it to be commited?

comment:8 in reply to: ↑ 6 Changed 16 months ago by gen3

Replying to exarkun:

Hello. Thanks for your contribution. Please read ReviewProcess to learn about the workflow we use for the issue tracker.

what should i add to my patch in order for it to be commited?

Changed 16 months ago by Chewy

comment:9 Changed 16 months ago by Chewy

  • Owner changed from gen3 to Chewy
  • Status changed from reopened to new

I added a more "review process compliant" patch, as this is my first commit too, please let me know how i'm doing.

comment:10 Changed 16 months ago by Chewy

  • Owner Chewy deleted

comment:11 Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to Chewy

Reviewing 6393.patch.

Thanks for your contribution.

  1. The test case and tests are missing docstrings (see #6301 for some hints on how to write them).
  2. The spacing between functions and classes doesn't follow the coding standard.
    • There is a missing space in delimiter,things[-1] which made it difficult for me to parse.
  3. Is there a reason to hard-code ', '? It seems like '; ' at least might occasionally be useful. Certainly defaulting to ', ' makes sense. (Maybe delimiter and finalDelimiter as arguments?)

Also, when preparing your path, can you generate it from the root of the source checkout (so the paths looks like twisted/dir/file.py rather than dir/file.py.

Changed 15 months ago by Chewy

comment:12 Changed 15 months ago by Chewy

  • Keywords review added
  • Owner Chewy deleted

Thank you for reviewing.
I attached a new version of the patch adressing your review.

comment:13 Changed 15 months ago by rwall

  • Owner set to rwall
  • Status changed from new to assigned

Reviewing....

comment:14 follow-up: Changed 15 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to Chewy
  • Status changed from assigned to new

Thanks Chewy for the latest patch.

There are still a few problems.

Please address or answer the following points:

  1. I personally don't like the name "itemizedList". I think it would be clearer to call it something like "listToSentence" or "listToHumanString".
    1. eg See http://apidock.com/rails/Array/to_sentence
  2. twisted/python/text.py
    1. Please add @param and @type and @returns epytext to the new function.
    2. If there is only one item in thing, you implicitly convert it to string, but not if there are multiple things which means that itemizedList([1,2,3]) will raise TypeError but itemizedList([1]) will not. I think you should call str on each of the items in things.
    3. If I pass in a string, it will itemize the characters of the string - which is probably confusing.
    4. If I pass a generator / iterator how should the function behave? I don't think it should be allowed. I think you should test "if not isinstance(things, (list, tuple)" and raise TypeError in that case.
    5. exarkun suggested looking at divmod itemizedStringList
      1. https://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Imaginary/imaginary/language.py#L299
      2. Join #twisted-dev and ask him to explain why that was written as a generator? Maybe there was a good reason and same approach should be used here.
  3. twisted/test/test_text.py
    1. Coding standard Issues (read https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html)
      1. Docstrings should not share the same line as the """
      2. 3 blank lines between top level module objects
      3. 2 blank lines between methods.
      4. No newline at end of file
    2. Add tests for handling of different valid things types (list or tuple)
    3. Add tests for expected failures when things is not a valid type. (assertRaises)
    4. Add test that things list items are converted to strings.

Please submit another patch for review after addressing or answering
the points above.

-RichardW.

comment:15 Changed 15 months ago by exarkun

I think 'One, Two and Three' is the wrong result for ["One", "Two", "Three"]. The correct result would be 'One, Two, and Three' (as given in the ticket description). See serial comma.

Join #twisted-dev and ask him to explain why that was written as a generator?

In its original context, it makes sense for it to be a generator. There's no compelling reason that the functionality must be implemented as a generator in Twisted.

Changed 15 months ago by Chewy

comment:16 in reply to: ↑ 14 Changed 15 months ago by Chewy

  • Keywords review added

Replying to rwall:

  1. I personally don't like the name "itemizedList". I think it would be clearer to call it something like "listToSentence" or "listToHumanString".

I went with 'toPhrase' for now, that seems the most semantically correct to me.

  1. If I pass in a string, it will itemize the characters of the string - which is probably confusing.
  2. If I pass a generator / iterator how should the function behave? I don't think it should be allowed. I think you should test "if not isinstance(things, (list, tuple)" and raise TypeError in that case.

I went with "if not hasattr(things, 'len') or isinstance(things, basestring)", do you think that's a good way to do so?

comment:17 Changed 15 months ago by Chewy

  • Owner Chewy deleted

comment:18 follow-up: Changed 15 months ago by exarkun

  • Keywords review removed
  • Owner set to Chewy

Hi. Thanks for your work on this ticket.

I went with "if not hasattr(things, 'len') or isinstance(things, basestring)", do you think that's a good way to do so?

hasattr is against the Twisted coding standard. The way to write what you meant is getattr(things, '__len__', None) is not None. Also, basestring is probably not something you ever want to use. As written, the function only works on "native strings" - bytes on Python 2, unicode on Python 3. This isn't a good idea for a new API. Pick a type and implement the function for that type. Once you've done this, it'll be easy: just reject inputs that are not of that type (and you can skip the __len__ check, I think).

Other things to note:

  1. There's some tension between this ticket and #6341. Perhaps consider putting this functionality somewhere else - maybe in trial's util module; maybe it's even a private helper only for trial.
  2. Use L{int} instead of C{int}. When you have the name of a thing, that's what L{} is for.
  3. An object that has a length and can be iterated over isn't exactly a type. If there is no well-defined type for an argument, then you can leave @type off. However, I think you should just say the type is L{list} in this case.
  4. Test method docstrings are sentences, they should end with a ".".
  5. The list in test_notString is missing some whitespace between its elements.
  6. Add an assertion about the text of the TypeError (and probably change the text of the TypeError after you change the documentation about the type of things).
  7. If the API stays public, it needs to be included in the module's __all__.

Thanks again.

Changed 15 months ago by Chewy

comment:19 in reply to: ↑ 18 Changed 15 months ago by Chewy

  • Component changed from core to trial
  • Keywords review added
  • Owner Chewy deleted
  1. There's some tension between this ticket and #6341. Perhaps consider putting this functionality somewhere else - maybe in trial's util module; maybe it's even a private helper only for trial.

You're right, i moved it to trial.util and made it 'private' since it makes more sense in the context of #6380.

  1. Use L{int} instead of C{int}. When you have the name of a thing, that's what L{} is for.

Is there some documentation somewhere about those? I couldn't find any...

comment:20 follow-up: Changed 14 months ago by tom.prince

  • Keywords review removed
  • Owner set to Chewy

There's some tension between this ticket and #6341. Perhaps consider putting this functionality somewhere else

Another way to resolve the tension would be to simply deprecate everything currently in twisted.python.text rather than the entire module. The module is being deprecated because stuff there has other (more standard?) implementations, or is quite specific.

You're right, i moved it to trial.util and made it 'private'

If it is going to live in trial, then it should be private, but see above.

since it makes more sense in the context of #6380.

I think the functionality is generally useful (which is why I suggested implementing it generically). #6380 was the motivation, but I doubt it is the only place where it would be useful.

Use L{int} instead of C{int}. When you have the name of a thing, that's what L{} is for.

Is there some documentation somewhere about those? I couldn't find any...

There is some stuff on #6318.

  1. (minor) Please use SynchronousTestCase, rather than TestCase, since you are testing anything asynchronous.
  2. (tricky) I find the fact that delimiter contains a trailing space, but `finalDelimiter doesn't contain a leading or trailing space confusing. I'm not sure what to suggest here, except this should at least be documented.
  3. It would be helpful to people using this, to give some suggestion of what to pass for finalDelimter (for example "and" and `"or" are likely choices).
  4. The docstring isn't entirely accurate, since delimiter won't be used if there are exactly two items.
  5. I find toPhrase is somewhat ambiguous. There are many things that could be turned into a phrase, and it is unclear which this does. Something like listToPhrase would be clearer.

Please resubmit for review after addressing the above points.

Changed 14 months ago by Chewy

comment:21 in reply to: ↑ 20 Changed 14 months ago by Chewy

  • Keywords review added
  • Owner Chewy deleted

I think the functionality is generally useful (which is why I suggested implementing it generically). #6380 was the motivation, but I doubt it is the only place where it would be useful.

I'm honestly a bit perplex about that issue. I left it as is for now, but this can still be done later.

Changed 13 months ago by jamesbroadhead

comment:22 Changed 13 months ago by jamesbroadhead

strips trailing whitespace from the current patch (including an additional case in trial/util.py)

comment:23 Changed 13 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/listToPhrase-6393

(In [39127]) Branching to listToPhrase-6393.

comment:24 Changed 13 months ago by tomprince

(In [39128]) Apply 6393.6.patch from jamesbroadhead.

Refs: #6393

comment:25 Changed 13 months ago by tom.prince

  • Author changed from tomprince to gen3, Chewy, jamesbroadhead
  • Keywords review removed

Thinking about it some more, I guess since the interface isn't entirely clear, it probably makes sense to keep this private, for the moment.

  1. There is a line longer than 80 characters, that needs to be wrapped (here)
  2. There is a test failure on python 3.
    • It appears to be an issue with the test, rather than the code.

Thanks for working on this. Please resubmit for review, with a patch against the branch, after wrapping the line, and fixing the test failure.

Changed 13 months ago by Chewy

comment:26 Changed 13 months ago by Chewy

  • Keywords review added

I'm not really familiar with how svn works so I hope i did this right.
Thank you for reviewing.

comment:27 Changed 13 months ago by tomprince

(In [39130]) Apply 6393.branch.patch from Chewy.

Refs: #6393

comment:28 Changed 13 months ago by tomprince

(In [39132]) Use str(error) for testing exception messages.

Refs: #6393, #6620

comment:29 Changed 13 months ago by tom.prince

  • Keywords review removed
  • Owner set to tom.prince

It appears that there isn't any documentation on how to test the string value of an exception. After some investigation, it appears that str(error) seems to be the most sensible way. I've filed #6220 to document this, and have updated the branch to reflect this.

I also changed the news file to misc, since this doesn't intoduce any public functions.

Other than that, this looks good. I'll merge once the buildbot is done.

Thanks everybody for your work on this.

comment:30 Changed 13 months ago by tomprince

  • Resolution set to fixed
  • Status changed from new to closed

(In [39135]) Merge listToPhrase-6393: Add a utility function to pretty-format list of things

Author: gen3, Chewy, jamesbroadhead
Reviewers: tom.prince, rwall, exarkun,
Fixes: #6393

When generating output to be displayed to a user, we should format
things grammatically. This provides a utility to format list of items
with proper separators and conjunctions.

Note: See TracTickets for help on using tickets.