Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-23984: Improve descriptor documentation #1034
Conversation
This comment has been minimized.
This comment has been minimized.
the-knights-who-say-ni
commented
Apr 7, 2017
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
the-knights-who-say-ni
added
the
CLA not signed
label
Apr 7, 2017
This comment has been minimized.
This comment has been minimized.
That doesn't seem correct. This will raise an |
DimitrisJim
reviewed
Apr 7, 2017
Hi @shubh3794 and thanks for the patch. Did you just remove the print call? This doesn't really improve the example (it makes it worse, actually) |
This comment has been minimized.
This comment has been minimized.
Also, if you can, please edit the title of the issue to include the b.p.o id by using: bpo-23984: "title of this issue" |
shubh3794
force-pushed the
shubh3794:doc_fix
branch
from
3a5195f
to
cf72358
Apr 7, 2017
shubh3794
changed the title
doc fixes for #23984
doc fixes as per bpo-23984
Apr 7, 2017
This comment has been minimized.
This comment has been minimized.
@DimitrisJim You're right, I was hasty while pushing it. Review now |
This comment has been minimized.
This comment has been minimized.
However one issue on the issue tracker was "that [the example] gives no hint of why one might want to use a staticmethod". And I don't see how this change improves that. Could you elaborate on that point? |
This comment has been minimized.
This comment has been minimized.
@MSeifert04 I thought about that, and I can explain the usage as well, but the usage can be thoroughly explained a paragraph, would that suffice or should I add a complete OO example(which I think is not required). Correct me if I'm wrong. |
This comment has been minimized.
This comment has been minimized.
I don't think it needs to be a "complete OO example" but just something "better" than returning the argument. Actually FWIW: I'm just a new contributor like you, so please don't assume I know what should be documented there. |
This comment has been minimized.
This comment has been minimized.
@MSeifert04 I think the usage of static method is more theoretical since practically people usually avoid static methods, And the purpose I think is clear from the method, that it can be called via a class or an object of that class. Use cases are just that it might fit into a context of a class which is already specified in the doc. |
This comment has been minimized.
This comment has been minimized.
@DimitrisJim Requesting your input over here. |
This comment has been minimized.
This comment has been minimized.
Using I'd suggest you stick with |
Mariatta
changed the title
doc fixes as per bpo-23984
bpo-23984: Improve descriptor documentation
Apr 10, 2017
Mariatta
added
the
type-documentation
label
Apr 10, 2017
serhiy-storchaka
approved these changes
Apr 10, 2017
serhiy-storchaka
requested a review
from
rhettinger
Apr 10, 2017
rhettinger
requested changes
Apr 11, 2017
I would rather leave the print() in the method call and instead remove the two print() calls on lines 365 and 367. |
rhettinger
self-assigned this
Apr 11, 2017
This comment has been minimized.
This comment has been minimized.
@rhettinger I have made the changes as you proposed. Kindly let me know if you want me to change anything else. |
Mariatta
removed
the
CLA not signed
label
Apr 13, 2017
the-knights-who-say-ni
added
the
CLA signed
label
Apr 13, 2017
This comment has been minimized.
This comment has been minimized.
@serhiy-storchaka @rhettinger Is the PR okay to be merged now, please let me know if I can make it better somehow |
terryjreedy
added
the
needs backport to 3.7
label
Mar 14, 2018
vstinner
removed
the
needs backport to 3.6
label
Jan 10, 2019
This comment has been minimized.
This comment has been minimized.
I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches |
This comment has been minimized.
This comment has been minimized.
@brettcannon Hey I had already made changes, I am awaiting review from @rhettinger. He suggested a few changes which I made, only his approval is pending. |
This comment has been minimized.
This comment has been minimized.
I have made the requested changes; please review again |
bedevere-bot
added
awaiting change review
and removed
awaiting changes
labels
Jan 11, 2019
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Jan 11, 2019
Thanks for making the requested changes! @serhiy-storchaka, @rhettinger: please review the changes made to this pull request. |
csabella
requested a review
from
rhettinger
Mar 4, 2019
csabella
added
the
skip news
label
Mar 18, 2019
rhettinger
approved these changes
Mar 18, 2019
bedevere-bot
added
awaiting merge
and removed
awaiting change review
labels
Mar 18, 2019
rhettinger
added
the
🤖 automerge
label
Mar 18, 2019
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 18, 2019
Sorry, I can't merge this PR. Reason: |
vstinner
closed this
Mar 20, 2019
vstinner
reopened this
Mar 20, 2019
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 20, 2019
@shubh3794: Status check is done, and it's a success |
1 similar comment
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 20, 2019
@shubh3794: Status check is done, and it's a success |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 20, 2019
Sorry, I can't merge this PR. Reason: |
1 similar comment
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 20, 2019
Sorry, I can't merge this PR. Reason: |
This comment has been minimized.
This comment has been minimized.
AppVeyor CI was stuck. I closed/reopened the PR to workaround that. |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 20, 2019
@shubh3794: Status check is done, and it's a success |
miss-islington
merged commit abbdd1f
into
python:master
Mar 20, 2019
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 20, 2019
Thanks @shubh3794 for the PR |
bedevere-bot
removed
the
awaiting merge
label
Mar 20, 2019
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Mar 20, 2019
GH-12459 is a backport of this pull request to the 3.7 branch. |
shubh3794 commentedApr 7, 2017
•
edited by bedevere-bot
https://bugs.python.org/issue23984