Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Document ClassVar #5733

Merged
merged 3 commits into from
Oct 16, 2018
Merged

Document ClassVar #5733

merged 3 commits into from
Oct 16, 2018

Conversation

nanjekyejoannah
Copy link
Contributor

This PR fixes #5546.

I have added a section in the class basics documentation with details on when, how and when not to use ClassVar.

cc @ilevkivskyi

@ilevkivskyi
Copy link
Member

I think @JukkaL might want to review this.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing more documentation! Left some comments -- mostly style/flow issues.

docs/source/class_basics.rst Outdated Show resolved Hide resolved
docs/source/class_basics.rst Outdated Show resolved Hide resolved
docs/source/class_basics.rst Show resolved Hide resolved
docs/source/class_basics.rst Show resolved Hide resolved
docs/source/class_basics.rst Show resolved Hide resolved
docs/source/class_basics.rst Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

Hey @nanjekyejoannah, do you want to push an updated version in response to Jukka's review? There's still time to get this cherry-picked in the upcoming 0.640 release.

Copy link
Contributor Author

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvanrossum Apologies for a late response. Been traveling for ages :). I have made the necessary changes. @JukkaL PTAL.

docs/source/class_basics.rst Show resolved Hide resolved
docs/source/class_basics.rst Outdated Show resolved Hide resolved
docs/source/class_basics.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! Looks good. There are a few things I'll probably tweak in a separate PR but this is fine as is.

@JukkaL JukkaL merged commit b67530e into python:master Oct 16, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document ClassVar
4 participants