bpo-23984: Improve descriptor documentation by shubh3794 · Pull Request #1034 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation31 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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 IndentationError: expected an indented block exception.
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"
doc fixes for #23984 doc fixes as per bpo-23984
@DimitrisJim You're right, I was hasty while pushing it. Review now
return is not a function, so generally you don't need the ().
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 staticmethod is rarely used, because Python (unlike Java, ...) doesn't enforce classes-only. So an example should at least hint at possible use-cases in Python. At least that's my interpretation of the comment
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.
Using return x looks like the safest option for the time being (mirroring the classmethod example later in the documentation). A little sentence under the example explaining why/where this is useful would probably be needed too.
I'd suggest you stick with return x for now and wait until @rhettinger, the author of this HOWTO, finds some time to come and offer his input on the matter. He'll judge the degree and nature of the change that's required here.
doc fixes as per bpo-23984 bpo-23984: Improve descriptor documentation
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.
@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
Sorry, I can't merge this PR. Reason: Required status check "continuous-integration/appveyor/pr" is expected..
@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: 2 of 4 required status checks are pending..
1 similar comment
Sorry, I can't merge this PR. Reason: 2 of 4 required status checks are pending..
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.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
rhettinger pushed a commit that referenced this pull request
