GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-41287: Handle `doc` argument of `property.__init__` in subclasses by sizmailov · Pull Request #23205 · python/cpython

Conversation

@sizmailov

@sizmailov sizmailov commented Nov 9, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Process explicit doc argument of property(...) in the same manner as docstring of getter function in property subclasses.

This eliminates behavior differences between dummy class Property(property): pass and property (see test case for example)

https://bugs.python.org/issue41287

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni

the-knights-who-say-ni commented Nov 9, 2020

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@sizmailov

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Nov 9, 2020
@bedevere-bot bedevere-bot added the awaiting review label Nov 9, 2020
@sizmailov
sizmailov force-pushed the fix-issue41287 branch 2 times, most recently from c33eb5a to 73e81e4 Compare November 9, 2020 13:14
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 9, 2020
@github-actions

github-actions Bot 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@sizmailov

sizmailov commented Dec 16, 2020

Copy link
Copy Markdown
Contributor Author

unstale
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Dec 17, 2020
@github-actions

github-actions Bot commented Jan 16, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Jan 16, 2021
MaxwellDupre
MaxwellDupre approved these changes Feb 8, 2022
View reviewed changes

@MaxwellDupre MaxwellDupre left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Works for me.
./python -m test -v test_pydoc
Ran 74 tests in 32.947s
OK (skipped=3)
test_pydoc passed in 33.1 sec
== Tests result: SUCCESS ==
1 test OK.
Total duration: 33.1 sec
Tests result: SUCCESS
cpython on  fix-issue41287 [$?] via 🐍 v3.11.0a5+ took 33s

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Feb 8, 2022
JelleZijlstra
JelleZijlstra reviewed Mar 24, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks, and sorry this took so long! Unfortunately this needs a merge conflict fixed now. It also needs a NEWS entry.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pydoc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra JelleZijlstra self-assigned this Mar 24, 2022
sizmailov added 2 commits April 3, 2022 11:04
@sizmailov
Handle doc argument of property.__init__ in subclasses
b19523a
@sizmailov
Add NEWS entry
0745f70
@sizmailov
sizmailov force-pushed the fix-issue41287 branch from 73e81e4 to 0745f70 Compare April 3, 2022 08:26
@sizmailov

sizmailov commented Apr 3, 2022

Copy link
Copy Markdown
Contributor Author

Rebased branch on current main, added NEWS entry, addressed review comments.
Hopefully CI would show no errors 🤞
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra self-requested a review April 3, 2022 14:29
JelleZijlstra
JelleZijlstra requested changes Apr 5, 2022
View reviewed changes
Comment thread Lib/test/test_pydoc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2022-04-03-11-25-02.bpo-41287.8CTdwf.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pydoc.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Apr 5, 2022
@bedevere-bot

bedevere-bot commented Apr 5, 2022

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sizmailov and others added 3 commits April 5, 2022 13:02
@sizmailov @JelleZijlstra
Update Misc/NEWS.d/next/Library/2022-04-03-11-25-02.bpo-41287.8CTdwf.rst
ebec602 
Co-authored-by: Jelle Zijlstra <[email protected]>
@sizmailov
Move test to test_property.py
f09e6ac
@sizmailov
Rename local get_doc -> prop_doc
dcab455
@sizmailov

sizmailov commented May 22, 2022

Copy link
Copy Markdown
Contributor Author

I think I found the leak. Can we run CI one more time, please?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 22, 2022
@bedevere-bot

bedevere-bot commented May 22, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit fb8c2d4 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 22, 2022
@sizmailov
Add a comment
f8fb254
sizmailov
sizmailov commented May 22, 2022
View reviewed changes

@sizmailov sizmailov left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Here are the last two things I want to add to this PR:

  1. add a comment about prop_doc and
  2. shortened version for the property branch.

I don't want to disrupt the current CI, so I'll push changes once it finishes.

EDIT: I pushed two more commits when CI reached 81 successful, 1 failure (same as in current main), and 1 stuck jobs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
sizmailov added 3 commits May 22, 2022 21:36
@sizmailov
Remove unnecessary check
fc16ab9
@sizmailov
Add test for property copy doc
4943868
@sizmailov
Assign getter_doc only for non-null prop_doc
36fc1b0
@sizmailov

sizmailov commented May 27, 2022

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra Could you please take another look at the PR :) This time everything should be in place.
Many thanks for the prompt CI trigger, it really helped to quickly check for errors, and saved my day!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra self-requested a review May 27, 2022 18:35
@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@bedevere-bot

bedevere-bot commented May 27, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 36fc1b0 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@JelleZijlstra

JelleZijlstra commented May 27, 2022

Copy link
Copy Markdown
Member

I triggered the buildbots to make sure, I'll review the PR again tonight. Thanks for your work!
sizmailov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra reviewed May 28, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good, just a few typos in the comments

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sizmailov reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread Lib/test/test_property.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_property.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_property.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_property.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_property.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_property.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@sizmailov @JelleZijlstra
Apply suggestions from code review
14bdd86 
Fix typos in comments

Co-authored-by: Jelle Zijlstra <[email protected]>
JelleZijlstra
JelleZijlstra approved these changes May 28, 2022
View reviewed changes
@JelleZijlstra
JelleZijlstra merged commit 66f5add into python:main May 29, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label May 29, 2022
@sizmailov sizmailov mannequin mentioned this pull request Apr 10, 2022
__doc__ attribute is not set in property-derived classes #85459
Closed
@JelleZijlstra

JelleZijlstra commented May 29, 2022

Copy link
Copy Markdown
Member

Congratulations on your first contribution to Python!
sizmailov and AlexWaygood reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henryiii henryiii mentioned this pull request Aug 3, 2022
[BUG]: Python 3.12 support pybind/pybind11#4115
Closed
3 tasks done
@Skylion007 Skylion007 mentioned this pull request Sep 16, 2022
fix: Python-3.12 compatibility pybind/pybind11#4168
Merged
@encukou

encukou commented Nov 3, 2022

Copy link
Copy Markdown
Member

This breaks subclasses that don't have a writable __doc__, see #98963
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou mentioned this pull request Nov 3, 2022
property assumes that its subclasses have __dict__ #98963
Closed
@gpshead

gpshead commented Jun 3, 2023

Copy link
Copy Markdown
Member

Please see #105262 which restores the behavior this broke.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markuspi markuspi mentioned this pull request Apr 21, 2026
Add docstrings for static fields and properties pybind/pybind11-stubgen#308
Merged
skarndev pushed a commit to pybind/pybind11-stubgen that referenced this pull request Apr 29, 2026
@markuspi
Add docstrings for static fields and properties (#308)
89a89a7 
This PR extracts docstrings from static fields defined via
`def_property_readonly_static` and `def_property_static`.
This is based on the observation that the docstring for these fields is
reachable from Python:

``` py
>>> demo._bindings.properties.WithPropDoc.def_property_readonly_static
0
>>> demo._bindings.properties.WithPropDoc.__dict__['def_property_readonly_static']
<pybind11_builtins.pybind11_static_property object at 0x7bd623d73a70>
>>> demo._bindings.properties.WithPropDoc.__dict__['def_property_readonly_static'].__doc__
'prop doc token'
```

For each `Field` detected by pybind11-stubgen,
`FixMissingFieldDocString` checks whether `__dict__[...]` is different
from `obj`, which should only be the case for static properties. It then
tries to use the dcostring in `__doc__`.

### Version compatibility

This approach requires cpython 3.12+ as it relies on
python/cpython#23205 and pybind11 2.10.1+ due to
pybind/pybind11#4168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
+1 more reviewer
@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@JelleZijlstra JelleZijlstra

Labels

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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@sizmailov @the-knights-who-say-ni @bedevere-bot @JelleZijlstra @rhettinger @encukou @gpshead @MaxwellDupre
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.