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.

Sign up for GitHub

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

Jump to bottom

gh-81087: Add str.dedent and cache it at compile time #13445

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

gh-81087: Add str.dedent and cache it at compile time #13445

wants to merge 12 commits into from
+1,048 −896

Conversation

remilapeyre
Copy link
Contributor

@remilapeyre remilapeyre commented May 20, 2019 •
edited by terryjreedy
Loading

@remilapeyre remilapeyre requested review from rhettinger and a team as code owners May 20, 2019 14:53
Carreau
Carreau reviewed May 21, 2019
View reviewed changes
Lib/textwrap.py Outdated Show resolved Hide resolved
gpshead
gpshead requested changes May 21, 2019
View reviewed changes
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing: A documentation update documenting the new str.dedent method.

Lib/test/test_configparser.py Outdated Show resolved Hide resolved
Lib/test/test_unicode.py Outdated Show resolved Hide resolved
Objects/unicodeobject.c Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tirkarthi
tirkarthi reviewed May 21, 2019
View reviewed changes
Lib/textwrap.py Outdated Show resolved Hide resolved
@csabella
Copy link
Contributor

@remilapeyre, if your changes are ready to be reviewed again, please add a PR-level comment as requested by @bedevere-bot. Thanks!

@remilapeyre
Copy link
Contributor Author

@csabella, sorry, I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

@csabella csabella requested a review from gpshead June 1, 2019 19:09
@csabella
Copy link
Contributor

@remilapeyre, please resolve the merge conflicts. Thank you!

@remilapeyre
Copy link
Contributor Author

Hi @csabella, it should be good now. There is a lot of PendingDeprecationWarnings in the tests. I'm not sure what I should do, should I fix all calls or let them as is for now?

@remilapeyre
Copy link
Contributor Author

Hi @csabella and @gpshead, I fix the conflicts and updated the Pull Request. Can you review again?

@gpshead
Copy link
Member

gpshead commented Nov 21, 2020

One thing I think would help this PR is not updating so many Lib/test/test_*.py files to replace their existing textwrap.dedent usage. It'd avoid the need for nearly so many conflict resolutions.

Changing those to use the new .dedent() method can be done in followups.

@gpshead gpshead self-assigned this Nov 21, 2020
@ammaraskar
Copy link
Member

@remilapeyre Would you like to untangle the code changes here from updating existing usages of textwrap.dedent like Gregory suggested or do you mind if someone else bases off this work with a Co-authored-by tag with you?

@remilapeyre
Copy link
Contributor Author

Hi @ammaraskar I don't mind either as I was seriously lacking time to take care of this the past few months. I'm trying to find half a day a week to work on CPython again so I should come back to this PR in a few days.

@rhettinger rhettinger removed their request for review May 5, 2022 04:18
@gpshead gpshead mentioned this pull request Jan 9, 2023
Open
hauntsaninja
hauntsaninja requested changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes as per above; we should separate the implementation of the feature from its use elsewhere in the codebase

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@terryjreedy terryjreedy changed the title bpo-36906: Add str.dedent and cache it at compile time gh-81087: Add str.dedent and cache it at compile time Feb 11, 2023
terryjreedy
terryjreedy requested changes Feb 11, 2023
View reviewed changes
Lib/idlelib/idle_test/htest.py
@@ -209,7 +208,7 @@ def _wrapper(parent): # htest #
_linenumbers_drag_scrolling_spec = {
'file': 'sidebar',
'kwds': {},
'msg': textwrap.dedent("""\
'msg': """\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having '.dedent moved from here to the end 16 lines down.

In any case, I want both idlelib changes removed from this PR to avoid gratuitous differences between versions.

@gpshead gpshead removed their assignment May 20, 2023
@gpshead gpshead added the stale Stale PR or inactive for long period of time. label May 20, 2023
@gpshead gpshead requested a review from methane July 23, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Carreau Carreau Carreau left review comments

@tirkarthi tirkarthi tirkarthi left review comments

@hauntsaninja hauntsaninja hauntsaninja requested changes

@terryjreedy terryjreedy terryjreedy requested changes

@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

@methane methane Awaiting requested review from methane

Assignees
No one assigned
Labels
awaiting changes stale Stale PR or inactive for long period of time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.