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

bpo-41287: Handle doc argument of property.__init__ in subclasses #23205

Merged
merged 19 commits into from
May 29, 2022

Conversation

sizmailov
Copy link
Contributor

@sizmailov sizmailov commented Nov 9, 2020

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

@the-knights-who-say-ni
Copy link

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!

@github-actions
Copy link

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-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@sizmailov
Copy link
Contributor Author

unstale

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Dec 17, 2020
@github-actions
Copy link

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

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 16, 2021
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a 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

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

@JelleZijlstra JelleZijlstra self-assigned this Mar 24, 2022
@sizmailov
Copy link
Contributor Author

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

@JelleZijlstra JelleZijlstra self-requested a review April 3, 2022 14:29
@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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 22, 2022
@sizmailov
Copy link
Contributor Author

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

@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 22, 2022
@bedevere-bot
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 22, 2022
Copy link
Contributor Author

@sizmailov sizmailov left a 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.

@sizmailov
Copy link
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!

@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
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@JelleZijlstra
Copy link
Member

I triggered the buildbots to make sure, I'll review the PR again tonight. Thanks for your work!

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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

Fix typos in comments

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra
Copy link
Member

Congratulations on your first contribution to Python!

@encukou
Copy link
Member

encukou commented Nov 3, 2022

This breaks subclasses that don't have a writable __doc__, see #98963

@gpshead
Copy link
Member

gpshead commented Jun 3, 2023

Please see #105262 which restores the behavior this broke.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants