Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-22702: Improves documentation for str.join method #156

Merged
merged 5 commits into from May 27, 2017

Conversation

Projects
None yet
10 participants
@CuriousLearner
Copy link
Contributor

commented Feb 18, 2017

@the-knights-who-say-ni

This comment has been minimized.

Copy link

commented Feb 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 the PSF 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:

  1. Sign the PSF contributor agreement
  2. Wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  3. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

@CuriousLearner CuriousLearner force-pushed the CuriousLearner:str_join_documentation branch from e419328 to 10a1cdf Feb 18, 2017

@DimitrisJim

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

Hi @CuriousLearner! The original text seems fine to me, why do you think this change is warranted?

@CuriousLearner

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2017

Hi @DimitrisJim !

As in the issue Raymond & others mentioned that at least the usage of repeated iterable word should be removed. Moreover, in general to make the docstring less verbose, this change is suggested.

Is there something else wanted in this PR?

@DimitrisJim

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2017

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 str provided in this method" seems more confusing than the original sentence.

@CuriousLearner

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2017

Sure, I've done the changes as suggested.

@CuriousLearner

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2017

CLA is signed and now visible in my account.

cc @ncoghlan

: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

This comment has been minimized.

Copy link
@zhangyangyu

zhangyangyu Feb 22, 2017

Member

IMHO *iterable* should be left, referring the parameter.

@wm75

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

I don't think anything should be changed here. The double iterable is much less disturbing when formatted as html.

@marco-buttu
Copy link
Contributor

left a comment

IMO it should be just "in iterable " and not "in the :term:iterable, as it is in the rest of the file when referring to the parameter iterable. Furthermore, you should apply the same modification to the bytearray.join() description, and add a second whitespace after the dot, before "A :exc:TypeError.

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2017

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

See http://bugs.python.org/issue22702#msg291366 for further discussion.

@CuriousLearner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

Hi @marco-buttu & @ncoghlan !

I've made the changes according to earlier review. Please review :)

@marco-buttu
Copy link
Contributor

left a comment

Just a minor change. The rest LGTM.

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

This comment has been minimized.

Copy link
@marco-buttu

marco-buttu Apr 13, 2017

Contributor

Could you please add an extra whitespace after the period? I mean, two whitespaces between objects. and The separator.

@CuriousLearner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2017

Hi @marco-buttu ! I've updated the patch. Can you please take a look?

@marco-buttu
Copy link
Contributor

left a comment

Thank you, ok to me :-)

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

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag May 10, 2017

Member

You dropped "the" before *iterable* in str.join() documentation, but kept it here. Is there a reason to keep it here?

This comment has been minimized.

Copy link
@CuriousLearner

CuriousLearner May 10, 2017

Author Contributor

@berkerpeksag I don't remember any specific reason for that. Should I include the in str.join() documentation?

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag May 10, 2017

Member

I'm not a native speaker so I'll defer the decision to @ncoghlan :)

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan May 10, 2017

Contributor

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.

This comment has been minimized.

Copy link
@CuriousLearner

CuriousLearner May 10, 2017

Author Contributor

@ncoghlan @berkerpeksag I've fixed it.

Is there something else needed for this PR?

@ncoghlan ncoghlan merged commit 08e2f35 into python:master May 27, 2017

2 checks passed

bedevere/issue-number Issue number 22702 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented May 28, 2017

Added the sprint label, as this PR was submitted at the PyCon Pune 2017 core development sprint.

Mariatta added a commit to Mariatta/cpython that referenced this pull request Jun 1, 2017

[3.6] bpo-22702: Clarify documentation of str.join & bytes.join (pyth…
…onGH-156)

The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition.
(cherry picked from commit 08e2f35)

Mariatta added a commit to Mariatta/cpython that referenced this pull request Jun 1, 2017

[3.5] bpo-22702: Clarify documentation of str.join & bytes.join (pyth…
…onGH-156)

The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition.
(cherry picked from commit 08e2f35)

Mariatta added a commit to Mariatta/cpython that referenced this pull request Jun 1, 2017

[2.7] bpo-22702: Clarify documentation of str.join & bytes.join (pyth…
…onGH-156)

The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition..
(cherry picked from commit 08e2f35)

Mariatta added a commit that referenced this pull request Jun 1, 2017

bpo-22702: Clarify documentation of str.join & bytes.join (GH-156) (G…
…H-1896)

The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition.
(cherry picked from commit 08e2f35)

Mariatta added a commit that referenced this pull request Jun 1, 2017

bpo-22702: Clarify documentation of str.join & bytes.join (GH-156) (G…
…H-1897)

The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition.
(cherry picked from commit 08e2f35)

Mariatta added a commit that referenced this pull request Jun 1, 2017

bpo-22702: Clarify documentation of str.join & bytes.join (GH-156) (G…
…H-1898)

The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition..
(cherry picked from commit 08e2f35)

mlouielu added a commit to mlouielu/cpython that referenced this pull request Jun 15, 2017

bpo-22702: Clarify documentation of str.join & bytes.join (pythonGH-156)
The "iterable iterable" phrasing created confusion between the term
reference and the parameter name.

This simplifies the phrasing to just use the parameter name
without linking directly to the term definition.

akruis pushed a commit to akruis/cpython that referenced this pull request Aug 16, 2018

Anselm Kruis
Stackless issue python#156: Fix "uncollectable objects at shutdown" w…
…arning

Since merging commit c1c47c1, the Stackless test case
test_shutdown.TestShutdown.test_kill_modifies_slp_cstack_chain emits the
warning: "gc:0: ResourceWarning: gc: 1 uncollectable objects at
shutdown; use gc.set_debug(gc.DEBUG_UNCOLLECTABLE) to list them".
This commit disables GC for this test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.