-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
bpo-23984: Improve descriptor documentation #1034
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
Conversation
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! |
That doesn't seem correct. This will raise an |
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.
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)
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" |
@DimitrisJim You're right, I was hasty while pushing it. Review now |
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? |
@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. |
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. |
@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. |
@DimitrisJim Requesting your input over here. |
Using I'd suggest you stick with |
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.
I would rather leave the print() in the method call and instead remove the two print() calls on lines 365 and 367.
@rhettinger I have made the changes as you proposed. Kindly let me know if you want me to change anything else. |
@serhiy-storchaka @rhettinger Is the PR okay to be merged now, please let me know if I can make it better somehow |
@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. |
I have made the requested changes; please review again |
Thanks for making the requested changes! @serhiy-storchaka, @rhettinger: please review the changes made to this pull request. |
Sorry, I can't merge this PR. Reason: |
@shubh3794: Status check is done, and it's a success ✅ . |
1 similar comment
@shubh3794: Status check is done, and it's a success ✅ . |
Sorry, I can't merge this PR. Reason: |
1 similar comment
Sorry, I can't merge this PR. Reason: |
AppVeyor CI was stuck. I closed/reopened the PR to workaround that. |
@shubh3794: Status check is done, and it's a success ✅ . |
Thanks @shubh3794 for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7. |
GH-12459 is a backport of this pull request to the 3.7 branch. |
https://bugs.python.org/issue23984 (cherry picked from commit abbdd1f) Co-authored-by: Shubham Aggarwal <shubham.aggarwal@zomato.com>
https://bugs.python.org/issue23984