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

gh-110383: Fix docstring for str.rsplit maxsplit param to 'starting from the right' #113353

Closed
wants to merge 2 commits into from

Conversation

sparrowt
Copy link
Contributor

@sparrowt sparrowt commented Dec 21, 2023

Previously the parameters including their docstrings were cloned from str.split but that says 'starting from the left' which is not correct for str.rsplit.

It is not possible to partially clone or modify according to: https://devguide.python.org/development-tools/clinic/#how-to-clone-existing-functions so copy from split and modify as necessary.

This follows on from #111247 hopefully addressing the feedback there.

Previously the parameters including their docstrings were cloned from `str.split` but that says 'starting from the left' which is not correct for `str.rsplit`.

It is not possible to partially clone or modify according to:
https://devguide.python.org/development-tools/clinic/#how-to-clone-existing-functions
so copy from `split` and modify as necessary.
@erlend-aasland
Copy link
Contributor

I propose a different approach in #113355.

@erlend-aasland
Copy link
Contributor

Thanks for your PR. While this approach fixes the problem, I hesitate with it slightly, since it duplicates a lot of lines. For example, we will now have to make sure both strip and rstrip are updated if the param docstring needs amendment in the future.

I propose instead to change the wording in the param docstring, and instead clarify how the two functions operate in the docstring body. I'm not sure which variant is clearest for the user.

@erlend-aasland
Copy link
Contributor

Thanks for your interest in improving CPython :)

@sparrowt
Copy link
Contributor Author

Fair enough, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants