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

Jump to bottom

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

Merged
merged 19 commits into from May 29, 2022
Merged

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

merged 19 commits into from May 29, 2022

Conversation

sizmailov
Copy link
Contributor

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

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

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

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!

@sizmailov sizmailov force-pushed the fix-issue41287 branch 2 times, most recently from c33eb5a to 73e81e4 Compare Nov 9, 2020
@github-actions
Copy link

@github-actions github-actions bot commented Dec 16, 2020

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 label Dec 16, 2020
@sizmailov
Copy link
Contributor Author

@sizmailov sizmailov commented Dec 16, 2020

unstale

@github-actions github-actions bot removed the stale label Dec 17, 2020
@github-actions
Copy link

@github-actions github-actions bot commented Jan 16, 2021

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

@github-actions github-actions bot added the stale label Jan 16, 2021
MaxwellDupre
MaxwellDupre approved these changes Feb 8, 2022
View changes
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

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

JelleZijlstra
JelleZijlstra reviewed Mar 24, 2022
View changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

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

Lib/test/test_pydoc.py Outdated Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra self-assigned this Mar 24, 2022
@sizmailov
Copy link
Contributor Author

@sizmailov sizmailov commented Apr 3, 2022

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

@JelleZijlstra JelleZijlstra self-requested a review Apr 3, 2022
JelleZijlstra
JelleZijlstra requested changes Apr 5, 2022
View changes
Lib/test/test_pydoc.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2022-04-03-11-25-02.bpo-41287.8CTdwf.rst Outdated Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
Lib/test/test_pydoc.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 5, 2022

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.

@sizmailov
Copy link
Contributor Author

@sizmailov sizmailov commented May 22, 2022

The latest commit passes tests locally. The only remaining thing is to check for refleaks. Could anyone trigger the CI?

@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots label May 22, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 22, 2022

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

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label May 22, 2022
@sizmailov
Copy link
Contributor Author

@sizmailov sizmailov commented May 22, 2022

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

@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots label May 22, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 22, 2022

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

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label May 22, 2022
sizmailov
sizmailov reviewed May 22, 2022
View changes
Copy link
Contributor Author

@sizmailov sizmailov left a comment •
edited

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.

Objects/descrobject.c Show resolved Hide resolved
Objects/descrobject.c Outdated Show resolved Hide resolved
@sizmailov
Copy link
Contributor Author

@sizmailov sizmailov commented May 27, 2022

@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
@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots label May 27, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 27, 2022

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

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label May 27, 2022
@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented May 27, 2022

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

JelleZijlstra
JelleZijlstra reviewed May 28, 2022
View changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Looks good, just a few typos in the comments

Lib/test/test_property.py Outdated Show resolved Hide resolved
Lib/test/test_property.py Outdated Show resolved Hide resolved
Lib/test/test_property.py Outdated Show resolved Hide resolved
Lib/test/test_property.py Outdated Show resolved Hide resolved
Lib/test/test_property.py Outdated Show resolved Hide resolved
Lib/test/test_property.py Outdated Show resolved Hide resolved
@sizmailov @JelleZijlstra
Apply suggestions from code review
14bdd86 
Fix typos in comments

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra JelleZijlstra merged commit 66f5add into python:main May 29, 2022
13 checks passed
@sizmailov sizmailov mannequin mentioned this pull request Apr 10, 2022
Closed
@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented May 29, 2022

Congratulations on your first contribution to Python!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

@MaxwellDupre MaxwellDupre

Assignees

@JelleZijlstra JelleZijlstra

Labels
CLA signed stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sizmailov @the-knights-who-say-ni @bedevere-bot @JelleZijlstra @rhettinger @MaxwellDupre