Utility function to pretty-format list of things

[cyli]

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.

[exarkun]

Some code ​in Imaginary might help with this.

[gen3]

the pretty print function in utils.py

[gen3]

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 :)

[exarkun]

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

[gen3]

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

[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?

[Chewy]

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

[tom.prince]

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.

[Chewy]

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

[rwall]

Reviewing....

[rwall]

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.

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

[Chewy]

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.

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.

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

[exarkun]

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.

[Chewy]

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

[tom.prince]

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.

[Chewy]

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.

[jamesbroadhead]

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

[tomprince]

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

[tomprince]

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

Refs: #6393

[tom.prince]

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.

[Chewy]

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

[tomprince]

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

Refs: #6393

[tomprince]

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

Refs: #6393, #6620

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

[tomprince]

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