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

bpo-23984: Improve descriptor documentation #1034

Merged
merged 3 commits into from Mar 20, 2019

Conversation

@shubh3794
Copy link
Contributor

shubh3794 commented Apr 7, 2017

@the-knights-who-say-ni

This comment has been minimized.

Copy link

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!

@MSeifert04

This comment has been minimized.

Copy link
Contributor

MSeifert04 commented Apr 7, 2017

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

@DimitrisJim
Copy link
Contributor

DimitrisJim left a comment

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

This comment has been minimized.

Copy link
Contributor

DimitrisJim commented Apr 7, 2017

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 force-pushed the shubh3794:doc_fix branch from 3a5195f to cf72358 Apr 7, 2017

@shubh3794 shubh3794 changed the title doc fixes for #23984 doc fixes as per bpo-23984 Apr 7, 2017

@shubh3794

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Apr 7, 2017

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

@MSeifert04

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Apr 7, 2017

@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

This comment has been minimized.

Copy link
Contributor

MSeifert04 commented Apr 7, 2017

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

This comment has been minimized.

Copy link
Contributor 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

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Apr 9, 2017

@DimitrisJim Requesting your input over here.

@DimitrisJim

This comment has been minimized.

Copy link
Contributor

DimitrisJim commented Apr 9, 2017

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

@rhettinger
Copy link
Contributor

rhettinger left a comment

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

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Apr 12, 2017

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

@shubh3794

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Apr 19, 2017

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

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 10, 2019

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

@shubh3794

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Jan 11, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

shubh3794 commented Jan 11, 2019

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Copy link

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 csabella requested a review from rhettinger Mar 4, 2019

@csabella csabella added the skip news label Mar 18, 2019

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Mar 18, 2019

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

This comment has been minimized.

Copy link

miss-islington commented Mar 20, 2019

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

1 similar comment
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Mar 20, 2019

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

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Mar 20, 2019

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

1 similar comment
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Mar 20, 2019

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

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Mar 20, 2019

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

@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Mar 20, 2019

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

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

5 checks passed

Azure Pipelines PR #20190320.18 succeeded
Details
bedevere/issue-number Issue number 23984 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Mar 20, 2019

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

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Mar 20, 2019

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Mar 20, 2019

bpo-23984: Improve descriptor documentation (pythonGH-1034)
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
You can’t perform that action at this time.