gh-112205: Support @getter annotation from AC#112396

corona10 · 2023-11-25T11:14:05Z

Issue: Support getters and setters in Argument Clinic #112205

corona10 · 2023-11-25T11:15:35Z

This is the first draft version of getter annotation for AC.
Please leave some feedback for changes, and then I will add test code at the end.

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

corona10 · 2023-11-25T13:48:38Z

Now logic for clinic.py become straightforward. PTAL :)

corona10 · 2023-11-25T14:34:08Z

And I am going to implementer @setter annotation with separate PR :)

Tools/clinic/clinic.py

corona10 · 2023-11-25T14:47:29Z

Tools/clinic/clinic.py

 """)
+GETTERDEF_PROTOTYPE_DEFINE: Final[str] =normalize_snippet(r"""
+ #define{methoddef_name} \
+{{"{name}",{methoddef_cast}{c_basename}{methoddef_cast_end}, NULL, NULL}},


We may need to reform the template for the setter case too. But not sure we should handle this at this PR :)

erlend-aasland · 2023-11-27T11:14:46Z

Thanks, Donghee! Can you also create a docs PR for the devguide?

corona10 · 2023-11-27T11:21:11Z

Thanks, Donghee! Can you also create a docs PR for the devguide?

Sure, Do you have any recommended section for this?

Tools/clinic/clinic.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

corona10 · 2023-11-27T11:47:32Z

@erlend-aasland @AlexWaygood
Here is the documentation PR: python/devguide#1232
For @setter, I will add it once the directive is implemented.

Tools/clinic/clinic.py

colesbury

This looks great. I don't have any comments on top of what Erlend and Alex wrote.

AlexWaygood

The Python code LGTM, thanks! As always, I'll leave review of the generated C code to the other two :)

corona10 · 2023-11-28T01:44:48Z

I will wait @erlend-aasland 1-2days :)

erlend-aasland · 2023-11-28T08:46:05Z

Tools/clinic/clinic.py

 parser_code: list[str] |None
-ifnotrequires_defining_class:
+iff.kindisGETTER:
+flags="NULL"


This looks strange to me; AFAICS, we don't use flags for getters (methoddef_flags is not included in the template). And if we were to supply a zero default for flags, it would have to be an int literal (0, not NULL).
Ideally we should not have to specify flags here, but a lot of the following code expect it to be defined. I suggest we either set it to the empty string or the zero int literal.
Suggested change
flags="NULL"
flags="0"
Suggested change
flags="NULL"
flags=""# This should end up unused
@AlexWaygood?

Ah please ignore my previous comment, now I understood what you want to say :)

You may already know: the reason I don't use methodef_flags to the template is due to the structure of getset.
cpython/Include/descrobject.h
Lines 11 to 17 in a194938
structPyGetSetDef{
constchar*name;
getterget;
setterset;
constchar*doc;
void*closure;
};

We're not creating a PyMethodDef struct here, so the template field naming is now confusing. For example, the methoddef_name and methoddef_cast names are just confusing when we're generating a PyGetSetDef struct. I think we should fix the naming sooner than later.

We're not creating a PyMethodDef struct here, so the template field naming is now confusing. For example, the methoddef_name and methoddef_cast names are just confusing when we're generating a PyGetSetDef struct. I think we should fix the naming sooner than later.
We need to refactor some of the code while implementing @setter, Can we handle this issue on the @setter PR? or Do you want to handle it as this PR?

dde933e
Remove unnecessary template :)

I'm not sure my suggested comment is good enough :) Wording it more explicit might be better (cc. @AlexWaygood).

I think this is fine... or, at least, I can't think of anything better :)
Looking at this function gives me a headache; this change doesn't really make my head hurt significantly more tbh :)

Yes, the urge to refactor is growing. I suggest we take a stab at it pretty soon, Alex :)

Tools/clinic/clinic.py

corona10 · 2023-11-29T23:59:39Z

@erlend-aasland If the current status is enough to be merged, I will merge this PR at this weekend :)
cc @AlexWaygood @colesbury

pythongh-112205: Support@Getterannotation from AC
72d9018

corona10 added the topic-free-threading label Nov 25, 2023

corona10 requested a review from colesbury November 25, 2023 11:14

corona10 requested review from AlexWaygood and erlend-aasland as code owners November 25, 2023 11:14

bedevere-appbot added the awaiting core review label Nov 25, 2023

bedevere-appbot mentioned this pull request Nov 25, 2023
Support getters and setters in Argument Clinic #112205
Closed

corona10 added the skip news label Nov 25, 2023

Update define macro
ea83973

AlexWaygood reviewed Nov 25, 2023
View reviewed changes

Tools/clinic/clinic.py Outdated Show resolvedHide resolved
Tools/clinic/clinic.py Outdated Show resolvedHide resolved

corona10and others added 2 commits November 25, 2023 20:58

Apply suggestions from code review
969adf9
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

nit
716b8d9

corona10 requested a review from AlexWaygood November 25, 2023 12:00

corona10 added 2 commits November 25, 2023 22:00

Add test case
c5fe2b2

Add test case
2d9800d

AlexWaygood reviewed Nov 25, 2023
View reviewed changes

Tools/clinic/clinic.py Outdated Show resolvedHide resolved

corona10and others added 5 commits November 25, 2023 22:29

Clean up
442c575

Update Tools/clinic/clinic.py
e0fb271
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Update logic
0778eb7

Update
d5b1751

Clean up
7f4bfa8

corona10 requested a review from AlexWaygood November 25, 2023 14:00

corona10 commented Nov 25, 2023
View reviewed changes

Tools/clinic/clinic.pyShow resolvedHide resolved

corona10 commented Nov 25, 2023
View reviewed changes

AlexWaygood reviewed Nov 27, 2023
View reviewed changes

Tools/clinic/clinic.py Outdated Show resolvedHide resolved

Update Tools/clinic/clinic.py
fe46de0
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood reviewed Nov 27, 2023
View reviewed changes

Tools/clinic/clinic.pyShow resolvedHide resolved

Address code review
0892d0d

colesbury reviewed Nov 27, 2023
View reviewed changes

AlexWaygood approved these changes Nov 27, 2023
View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting core review labels Nov 27, 2023

erlend-aasland reviewed Nov 28, 2023
View reviewed changes

Address code review
769f6cd

corona10 requested a review from erlend-aasland November 28, 2023 13:39

corona10 added 2 commits November 28, 2023 23:07

Address code review
dde933e

Change template key
d381b5e

erlend-aasland reviewed Nov 28, 2023
View reviewed changes

Tools/clinic/clinic.py Outdated Show resolvedHide resolved

corona10 added 2 commits November 29, 2023 06:51

Use Py_UNUSED for context
351ba08

Update test
f53a035

erlend-aasland approved these changes Nov 30, 2023
View reviewed changes

corona10 merged commit 7eeea13 into python:mainNov 30, 2023

bedevere-appbot removed the awaiting merge label Nov 30, 2023

corona10 deleted the gh-112205 branch November 30, 2023 10:40

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
pythongh-112205: Support@Getterannotation from AC (pythongh-112396)
fae9ef6

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
pythongh-112205: Support@Getterannotation from AC (pythongh-112396)
9bbf3ee

	structPyGetSetDef{
	constchar*name;
	getterget;
	setterset;
	constchar*doc;
	void*closure;
	};

Uh oh!

gh-112205: Support @getter annotation from AC#112396

gh-112205: Support @getter annotation from AC #112396

Uh oh!

Conversation

corona10 commented Nov 25, 2023• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Nov 25, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Nov 25, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Nov 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Nov 27, 2023

Uh oh!

corona10 commented Nov 27, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 commented Nov 27, 2023

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10 commented Nov 29, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

corona10 commented Nov 25, 2023•
edited by bedevere-app bot
Loading

corona10 commented Nov 25, 2023•
edited
Loading

corona10 commented Nov 25, 2023•
edited
Loading

corona10 commented Nov 27, 2023•
edited
Loading

corona10 commented Nov 29, 2023•
edited
Loading