Skip to content

gh-86457: Fix signature for code.replace(). - #23199

Merged
serhiy-storchaka merged 7 commits into
python:mainfrom
serhiy-storchaka:clinic-code-replace
Aug 7, 2023
Merged

gh-86457: Fix signature for code.replace().#23199
serhiy-storchaka merged 7 commits into
python:mainfrom
serhiy-storchaka:clinic-code-replace

Conversation

@serhiy-storchaka

@serhiy-storchakaserhiy-storchaka commented Nov 8, 2020•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@serhiy-storchaka
serhiy-storchaka marked this pull request as draft November 8, 2020 10:40
@serhiy-storchaka
serhiy-storchaka removed the request for review from tiranNovember 8, 2020 10:42
@github-actions

github-actionsBot commented Dec 16, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Jul 31, 2022
iritkatriel
iritkatriel previously requested changes Dec 6, 2022
View reviewed changes

@iritkatrieliritkatriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has merge conflicts now.

@bedevere-bot

bedevere-bot commented Dec 6, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

iritkatriel
iritkatriel reviewed Dec 6, 2022
View reviewed changes
Comment threadObjects/codeobject.c Outdated
@netlify

netlifyBot commented Dec 7, 2022•
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview ready!

NameLink
🔨 Latest commit748a7e8
🔍 Latest deploy loghttps://app.netlify.com/sites/python-cpython-preview/deploys/6390d92ee0f8950008cc428e
😎 Deploy Previewhttps://deploy-preview-23199--python-cpython-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Dec 7, 2022
@serhiy-storchaka
pythongh-23199: Fix signature for code.replace().
0dfd286
@serhiy-storchaka
serhiy-storchaka marked this pull request as ready for review December 7, 2022 14:06
@iritkatriel
iritkatriel dismissed their stale reviewDecember 7, 2022 14:25

conflicts were resolved.

iritkatriel
iritkatriel reviewed Dec 7, 2022
View reviewed changes
Comment threadTools/clinic/clinic.py Outdated
iritkatriel
iritkatriel approved these changes Dec 7, 2022
View reviewed changes

@iritkatrieliritkatriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand the clinic code, but the rest LGTM.

@serhiy-storchakaserhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jun 29, 2023
@AlexWaygood
AlexWaygood dismissed iritkatriel’s stale reviewJune 29, 2023 14:38

merge conflicts were fixed

@erlend-aasland

erlend-aasland commented Jun 29, 2023

Copy link
Copy Markdown
Contributor

I will have a look, but it may take some days before I find time. Thanks for the ping, @serhiy-storchaka

serhiy-storchaka
serhiy-storchaka commented Aug 7, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py
if self.forced_text_signature:
add(self.forced_text_signature)
else:
add('(')

@serhiy-storchakaserhiy-storchakaAug 7, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

All following lines just increase indentation. GitHub is not good in showing such case.

@erlend-aaslanderlend-aaslandAug 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the record, I plan to refactor the signature generation into a helper function. See #107623

@erlend-aasland

erlend-aasland commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

It would be better if Argument Clinic could handle this by itself, without the need for the @text_signature override. However, this is a good workaround. It also unblocks cases in the code base where we could not apply clinic because of signature issues.

@erlend-aaslanderlend-aasland changed the title bpo-42291: Fix signature for code.replace().gh-86457: Fix signature for code.replace().Aug 7, 2023
@serhiy-storchaka
serhiy-storchaka merged commit 0e6e32f into python:mainAug 7, 2023
@miss-islington

miss-islington commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@serhiy-storchaka
serhiy-storchaka deleted the clinic-code-replace branch August 7, 2023 20:34
@miss-islington

miss-islington commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0e6e32fb84b2f7cb668e0b9927637587081e38cd 3.12

@miss-islington

miss-islington commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0e6e32fb84b2f7cb668e0b9927637587081e38cd 3.11

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Aug 7, 2023
@serhiy-storchaka
[3.12]pythongh-86457: Fix signature for code.replace() (pythonGH-23199)
88cfea3
Also add support of @text_signature in Argument Clinic..
(cherry picked from commit 0e6e32f)
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchakaserhiy-storchaka mentioned this pull request Apr 10, 2022
Closed
@bedevere-bot

bedevere-bot commented Aug 7, 2023

Copy link
Copy Markdown

GH-107744 is a backport of this pull request to the 3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12 only security fixes label Aug 7, 2023
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Aug 7, 2023
@serhiy-storchaka
[3.11]pythongh-86457: Fix signature for code.replace() (pythonGH-23199)
3818723
Also add support of @text_signature in Argument Clinic..
(cherry picked from commit 0e6e32f)
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot

bedevere-bot commented Aug 7, 2023

Copy link
Copy Markdown

GH-107746 is a backport of this pull request to the 3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11 only security fixes label Aug 7, 2023
@serhiy-storchaka

serhiy-storchaka commented Aug 7, 2023

Copy link
Copy Markdown
MemberAuthor

I am sorry for creating conflicts with your PR, but this PR was planned for backporting, and your refactoring seems is not purposed for backporting.

@erlend-aasland

erlend-aasland commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

I am sorry for creating conflicts with your PR, but this PR was planned for backporting, and your refactoring seems is not purposed for backporting.

No worries; I'll handle that :)

serhiy-storchaka added a commit that referenced this pull request Aug 9, 2023
@serhiy-storchaka
[3.11]gh-86457: Fix signature for code.replace() (GH-23199) (GH-107746)
edaa0db
Also add support of @text_signature in Argument Clinic.
(cherry picked from commit 0e6e32f)
Yhg1s pushed a commit that referenced this pull request Aug 16, 2023
@serhiy-storchaka
[3.12]gh-86457: Fix signature for code.replace() (GH-23199) (#107744)
9864f9a
* [3.12] gh-86457: Fix signature for code.replace() (GH-23199)
Also add support of @text_signature in Argument Clinic..
(cherry picked from commit 0e6e32f)
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
* Update 2023-08-07-16-30-48.gh-issue-95065.-im4R5.rst
@serhiy-storchakaserhiy-storchaka removed their assignment Sep 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatrieliritkatrieliritkatriel left review comments
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner
@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner
@AlexWaygoodAlexWaygoodAwaiting requested review from AlexWaygood

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@serhiy-storchaka@bedevere-bot@erlend-aasland@miss-islington@iritkatriel@the-knights-who-say-ni@ezio-melotti