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

Add "how to" for the setter Argument Clinic directive #1245

Merged
merged 17 commits into from
Dec 13, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 10, 2023

development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
@corona10 corona10 requested a review from hugovk December 11, 2023 06:50
development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor

Hm, instead of having two how-to's -- one for @getter and one for @setter -- how about unifying them and creating a "How to declare PyGetSetDef functions"? After all, Argument Clinic's audience is (mainly) developers who are fairly comfortable with the C API.

@corona10 corona10 closed this Dec 11, 2023
@corona10 corona10 reopened this Dec 11, 2023
@AlexWaygood
Copy link
Member

(I'm travelling right now, but will do my best to review this tomorrow or Wednesday!)

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

I left some suggestions.

development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! Some minor points:

development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
development-tools/clinic.rst Outdated Show resolved Hide resolved
Comment on lines 2040 to 2041
And then the implementation will work the same as a Python method which is
decorated by :py:class:`property`.
Copy link
Member

@AlexWaygood AlexWaygood Dec 13, 2023

Choose a reason for hiding this comment

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

Not sure this should be deleted; the Python REPL example at the bottom now has no introduction explaining what it's illustrating

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I will supplement to the above descriptions.

Copy link
Member Author

@corona10 corona10 Dec 13, 2023

Choose a reason for hiding this comment

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

Hmm.. I think that "Getters" and "setters" are C functions defined in a :c:type:`PyGetSetDef` struct that facilitate :py:class:`property`-like access for a class will be enough for understand Python REPL..

@corona10 corona10 requested a review from AlexWaygood December 13, 2023 13:46
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@corona10 corona10 merged commit 47d44b3 into python:main Dec 13, 2023
2 checks passed
@corona10 corona10 deleted the ac-setter branch December 13, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants