Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
bpo-22702: Improves documentation for str.join method#156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedFeb 18, 2017
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:
Thanks again to your contribution and we look forward to looking at it! |
e419328 to10a1cdfCompareHi@CuriousLearner! The original text seems fine to me, why do you think this change is warranted? |
Hi@DimitrisJim ! As in the issue Raymond & others mentioned that at least the usage of repeated Is there something else wanted in this PR? |
DimitrisJim commentedFeb 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes the double use of iterable is odd here, my view is that that is the only thing needing change. My main issue is that the sentence "separated by the string |
Sure, I've done the changes as suggested. |
CLA is signed and now visible in my account. |
Doc/library/stdtypes.rst Outdated
| :term:`iterable` *iterable*. A :exc:`TypeError` will be raised if there are | ||
| any non-string values in *iterable*, including :class:`bytes` objects. The | ||
| separator between elements is the string providing this method. | ||
| :term:`iterable`. A :exc:`TypeError` will be raised if there are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO *iterable* should be left, referring the parameter.
wm75 commentedMar 1, 2017
I don't think anything should be changed here. The double iterable is much less disturbing when formatted as html. |
marco-buttu left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO it should be just "initerable " and not "in the :term:iterable, as it is in the rest of the file when referring to the parameteriterable. Furthermore, you should apply the same modification to thebytearray.join() description, and add a second whitespace after the dot, before "A :exc:TypeError.
@CuriousLearner Sorry for the delayed review, but I'm inclined to agree with@marco-buttu here - the iterable to drop is the term reference, rather than the parameter name. Seehttp://bugs.python.org/issue22702#msg291366 for further discussion. |
Hi@marco-buttu &@ncoghlan ! I've made the changes according to earlier review. Please review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a minor change. The rest LGTM.
Doc/library/stdtypes.rst Outdated
| separator between elements is the string providing this method. | ||
| Return a string which is the concatenation of the strings in *iterable*. | ||
| A :exc:`TypeError` will be raised if there are any non-string values in | ||
| *iterable*, including :class:`bytes` objects. The separator between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you please add an extra whitespace after the period? I mean, two whitespaces betweenobjects. andThe separator.
Hi@marco-buttu ! I've updated the patch. Can you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you, ok to me :-)
Doc/library/stdtypes.rst Outdated
| that are not :term:`bytes-like objects <bytes-like object>`, including | ||
| :class:`str` objects. The separator between elements is the contents | ||
| of the bytes or bytearray object providing this method. | ||
| binary data sequences in the *iterable*. A :exc:`TypeError` will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You dropped "the" before *iterable* in str.join() documentation, but kept it here. Is there a reason to keep it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@berkerpeksag I don't remember any specific reason for that. Should I includethe instr.join() documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not a native speaker so I'll defer the decision to@ncoghlan :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We want to drop it - "iterable" refers specifically to the method parameter rather than the general term, so it doesn't need "the" in front of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ncoghlan@berkerpeksag I've fixed it.
Is there something else needed for this PR?
Added the sprint label, as this PR was submitted at the PyCon Pune 2017 core development sprint. |
…arningSince merging commit c1c47c1, the Stackless test casetest_shutdown.TestShutdown.test_kill_modifies_slp_cstack_chain emits thewarning: "gc:0: ResourceWarning: gc: 1 uncollectable objects atshutdown; use gc.set_debug(gc.DEBUG_UNCOLLECTABLE) to list them".This commit disables GC for this test.
Use pre-existing info instead of making another API call.python/miss-islington#153
Fixes issuehttp://bugs.python.org/issue22702