Opened 9 years ago
Closed 9 years ago
#6393 enhancement closed fixed (fixed)
Utility function to pretty-format list of things
Reported by: | Ying Li | Owned by: | Tom Prince |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | trial | Keywords: | easy |
Cc: | Francis Sylvain | Branch: |
branches/listToPhrase-6393
branch-diff, diff-cov, branch-cov, buildbot |
Author: | gen3, Chewy, jamesbroadhead |
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)
Change History (38)
comment:1 Changed 9 years ago by
Keywords: | easy added |
---|
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
Cc: | Francis Sylvain added |
---|---|
Owner: | set to Francis Sylvain |
Status: | new → assigned |
comment:4 Changed 9 years ago by
Owner: | changed from Francis Sylvain to gen3 |
---|---|
Status: | assigned → new |
Changed 9 years ago by
Attachment: | pretty_print_6393.patch added |
---|
the pretty print function in utils.py
comment:5 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → 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: 8 Changed 9 years ago by
Keywords: | review added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Hello. Thanks for your contribution. Please read ReviewProcess to learn about the workflow we use for the issue tracker.
comment:8 Changed 9 years ago by
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 9 years ago by
Attachment: | 6393.patch added |
---|
comment:9 Changed 9 years ago by
Owner: | changed from gen3 to Francis Sylvain |
---|---|
Status: | reopened → 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 9 years ago by
Owner: | Francis Sylvain deleted |
---|
comment:11 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Francis Sylvain |
Reviewing 6393.patch.
Thanks for your contribution.
- The test case and tests are missing docstrings (see #6301 for some hints on how to write them).
- 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.
- There is a missing space in
- Is there a reason to hard-code
', '
? It seems like'; '
at least might occasionally be useful. Certainly defaulting to', '
makes sense. (Maybedelimiter
andfinalDelimiter
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 9 years ago by
Attachment: | 6393.2.patch added |
---|
comment:12 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Francis Sylvain deleted |
Thank you for reviewing. I attached a new version of the patch adressing your review.
comment:14 follow-up: 16 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Francis Sylvain |
Status: | assigned → new |
Thanks Chewy for the latest patch.
There are still a few problems.
Please address or answer the following points:
- I personally don't like the name "itemizedList". I think it would be clearer to call it something like "listToSentence" or "listToHumanString".
- twisted/python/text.py
- Please add @param and @type and @returns epytext to the new function.
- 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.
- If I pass in a string, it will itemize the characters of the string - which is probably confusing.
- 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.
- exarkun suggested looking at divmod itemizedStringList
- https://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Imaginary/imaginary/language.py#L299
- 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.
- twisted/test/test_text.py
- Coding standard Issues (read https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html)
- Docstrings should not share the same line as the """
- 3 blank lines between top level module objects
- 2 blank lines between methods.
- No newline at end of file
- Add tests for handling of different valid things types (list or tuple)
- Add tests for expected failures when things is not a valid type. (assertRaises)
- Add test that things list items are converted to strings.
- Coding standard Issues (read https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html)
Please submit another patch for review after addressing or answering the points above.
-RichardW.
comment:15 Changed 9 years ago by
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 9 years ago by
Attachment: | 6393.3.patch added |
---|
comment:16 Changed 9 years ago by
Keywords: | review added |
---|
Replying to rwall:
- 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.
- If I pass in a string, it will itemize the characters of the string - which is probably confusing.
- 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 9 years ago by
Owner: | Francis Sylvain deleted |
---|
comment:18 follow-up: 19 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Francis Sylvain |
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:
- 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.
- Use
L{int}
instead ofC{int}
. When you have the name of a thing, that's whatL{}
is for. - 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 isL{list}
in this case. - Test method docstrings are sentences, they should end with a ".".
- The list in
test_notString
is missing some whitespace between its elements. - Add an assertion about the text of the
TypeError
(and probably change the text of theTypeError
after you change the documentation about the type ofthings
). - If the API stays public, it needs to be included in the module's
__all__
.
Thanks again.
Changed 9 years ago by
Attachment: | 6393.4.patch added |
---|
comment:19 Changed 9 years ago by
Component: | core → trial |
---|---|
Keywords: | review added |
Owner: | Francis Sylvain deleted |
- 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.
- Use
L{int}
instead ofC{int}
. When you have the name of a thing, that's whatL{}
is for.
Is there some documentation somewhere about those? I couldn't find any...
comment:20 follow-up: 21 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Francis Sylvain |
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 ofC{int}
. When you have the name of a thing, that's whatL{}
is for.Is there some documentation somewhere about those? I couldn't find any...
There is some stuff on #6318.
- (minor) Please use SynchronousTestCase, rather than TestCase, since you are testing anything asynchronous.
- (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. - 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). - The docstring isn't entirely accurate, since
delimiter
won't be used if there are exactly two items. - 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 likelistToPhrase
would be clearer.
Please resubmit for review after addressing the above points.
Changed 9 years ago by
Attachment: | 6393.5.patch added |
---|
comment:21 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Francis Sylvain 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 9 years ago by
Attachment: | 6393.6.patch added |
---|
comment:22 Changed 9 years ago by
strips trailing whitespace from the current patch (including an additional case in trial/util.py)
comment:23 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/listToPhrase-6393 |
(In [39127]) Branching to listToPhrase-6393.
comment:25 Changed 9 years ago by
Author: | tomprince → 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.
- There is a line longer than 80 characters, that needs to be wrapped (here)
- 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 9 years ago by
Attachment: | 6393.branch.patch added |
---|
comment:26 Changed 9 years ago by
Keywords: | review added |
---|
I'm not really familiar with how svn works so I hope i did this right. Thank you for reviewing.
comment:28 Changed 9 years ago by
comment:29 Changed 9 years ago by
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 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → 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.
Some code in Imaginary might help with this.