Skip to content

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

Merged
merged 3 commits into from
Mar 20, 2019

Conversation

shubh3794
Copy link

@shubh3794 shubh3794 commented Apr 7, 2017

@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 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!

@MSeifert04
Copy link
Contributor

That doesn't seem correct. This will raise an IndentationError: expected an indented block exception.

Copy link
Contributor

@DimitrisJim DimitrisJim left a 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)

@DimitrisJim
Copy link
Contributor

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 shubh3794 changed the title doc fixes for #23984 doc fixes as per bpo-23984 Apr 7, 2017
@shubh3794
Copy link
Author

@DimitrisJim You're right, I was hasty while pushing it. Review now

@MSeifert04
Copy link
Contributor

MSeifert04 commented Apr 7, 2017

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?

@shubh3794
Copy link
Author

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

@MSeifert04
Copy link
Contributor

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.

@shubh3794
Copy link
Author

shubh3794 commented Apr 9, 2017

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

@shubh3794
Copy link
Author

@DimitrisJim Requesting your input over here.

@DimitrisJim
Copy link
Contributor

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.

@Mariatta Mariatta changed the title doc fixes as per bpo-23984 bpo-23984: Improve descriptor documentation Apr 10, 2017
@Mariatta Mariatta added the docs Documentation in the Doc dir label Apr 10, 2017
Copy link
Contributor

@rhettinger rhettinger left a 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 rhettinger self-assigned this Apr 11, 2017
@shubh3794
Copy link
Author

@rhettinger I have made the changes as you proposed. Kindly let me know if you want me to change anything else.

@shubh3794
Copy link
Author

@serhiy-storchaka @rhettinger Is the PR okay to be merged now, please let me know if I can make it better somehow

@shubh3794
Copy link
Author

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

@shubh3794
Copy link
Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@serhiy-storchaka, @rhettinger: please review the changes made to this pull request.

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Required status check "continuous-integration/appveyor/pr" is expected..

@vstinner vstinner closed this Mar 20, 2019
@vstinner vstinner reopened this Mar 20, 2019
@miss-islington
Copy link
Contributor

@shubh3794: Status check is done, and it's a success ✅ .

1 similar comment
@miss-islington
Copy link
Contributor

@shubh3794: Status check is done, and it's a success ✅ .

@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: 2 of 4 required status checks are pending..

1 similar comment
@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: 2 of 4 required status checks are pending..

@vstinner
Copy link
Member

AppVeyor CI was stuck. I closed/reopened the PR to workaround that.

@miss-islington
Copy link
Contributor

@shubh3794: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit abbdd1f into python:master Mar 20, 2019
@miss-islington
Copy link
Contributor

Thanks @shubh3794 for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-12459 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 20, 2019
https://bugs.python.org/issue23984
(cherry picked from commit abbdd1f)

Co-authored-by: Shubham Aggarwal <shubham.aggarwal@zomato.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.