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-40304: Correct type(name, bases, dict) doc #19553

Merged
merged 9 commits into from Jan 22, 2021

Conversation

verhovsky
Copy link
Contributor

@verhovsky verhovsky commented Apr 16, 2020

@verhovsky
Copy link
Contributor Author

Please correct me if there's some gotcha I'm missing, but if classes inherit from object, then don't they also do that when created via type?

>>> type('X', (object,), dict(a=1)).__bases__
(<class 'object'>,)
>>> type('X', (), dict(a=1)).__bases__
(<class 'object'>,)

@verhovsky verhovsky changed the title Remove unnecessary object base class from type docs bpo-40304: remove unnecessary object base class from type docs Apr 16, 2020
@merwok
Copy link
Member

merwok commented Apr 17, 2020

No gotcha, in Python 3 both are equivalent.

This means that this bit of the docs (a couple paragraphs before your change) is not strictly accurate:

the bases class name and becomes the :attr:~definition.__name__ attribute; the bases tuple itemizes the base classes and becomes the :attr:~class.__bases__ tuple itemizes the base classes and becomes the :attr:~class.__bases__ attribute

object ends up in __bases__ even when implicitly added by the metaclass!

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

E'ric, can you finish this off, with blurb and possible additional doc change?

@merwok merwok self-assigned this Apr 30, 2020
@taleinat
Copy link
Contributor

Ping, @merwok?

@terryjreedy terryjreedy added needs backport to 3.9 only security fixes and removed needs backport to 3.7 labels Sep 27, 2020
@merwok
Copy link
Member

merwok commented Sep 27, 2020

Hi! I will come back to this.

@merwok merwok changed the title bpo-40304: remove unnecessary object base class from type docs bpo-40304: correct some notes in type doc Sep 30, 2020
@merwok
Copy link
Member

merwok commented Sep 30, 2020

@taleinat @terryjreedy Please review additional changes!

  • class dict does not become instance __dict__ at all
  • itemizes is an obscure term, I thought about using enumerates but went for the simpler contains
  • I think this does not need a NEWS note, unless this is the first contribution from the PR author in which case I will add one to give attribution as a thank-you 🙂

@merwok
Copy link
Member

merwok commented Oct 19, 2020

I have added the skip news and backport labels. If there is no negative review about the wording I will merge this in a couple days!

Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

This is an important fix and I like the direction!

However, the new wording is still not accurate: When providing one or more base classes, object is not added to __bases__. For example:

>>> class A:
	pass
>>> type('B', (A,), {}).__bases__
(<class '__main__.A'>,)

Also, if the attributes dict doesn't become __dict__, what exactly is done with it? Perhaps this would be a good place for a link to the docs about class instantiation?

Doc/library/functions.rst Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@merwok
Copy link
Member

merwok commented Oct 20, 2020

Also, if the attributes dict doesn't become dict, what exactly is done with it?

It is passed to the metaclass so that functions become methods, other descriptors do their thing, and all attributes are stored in the class object in the end… in TheClass.__dict__! (instance __dict__ is only for instance attributes)

@merwok
Copy link
Member

merwok commented Oct 20, 2020

Text I removed:

the dict dictionary is the namespace containing definitions
for class body and is copied to a standard dictionary to become the
:attr:~object.__dict__ attribute.

so it was not wrong but could be confusing; what do you think about this change:

to become the :attr:~object.__dict__ attribute of the class object.

(let me check if the link target only talks about class instances)

@terryjreedy terryjreedy self-assigned this Jan 21, 2021
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

LGTM. Small suggestion about the NEWS entry, @terryjreedy.

Doc/library/functions.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

terryjreedy and others added 2 commits January 21, 2021 17:00
Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
…LK7Ps.rst

Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
@terryjreedy terryjreedy dismissed taleinat’s stale review January 21, 2021 22:04

Made requested changes

:attr:`~class.__bases__` attribute; if empty, :class:`object`, the
ultimate base of all classes, is added. The *dict* dictionary contains
attribute and method definitions for the class body; it may be copied
or wrapped before becoming the :attr:`~class.__dict__` attribute.
Copy link
Member

@merwok merwok Jan 21, 2021

Choose a reason for hiding this comment

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

Where does this link go?

class.__bases__ is at https://docs.python.org/3/library/stdtypes.html#class.__bases__
does class.__dict__ go to https://docs.python.org/3/reference/datamodel.html#index-48 ?

Copy link
Member

Choose a reason for hiding this comment

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

No link is created!

Ideas:

  • keep object.__dict__ but amend the doc there (in library/stdtypes) to describe __dict__ for class objects
  • change the reference to be to __dict__ (class attribute)

Copy link
Member

Choose a reason for hiding this comment

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

Since I changed object to class and that does not work, I changed it back for this issue, so it continues to link to
file:///F:/dev/3x/Doc/build/html/library/stdtypes.html#object.dict
Since 'objects' include 'classes', the current text is not incorrect. I consider changing it a separate issue.
If classes were the only objects with dict attribute (I am not sure), and object.dict should be class.dict, a change would require grepping for other references to change. This would be a different issue also.

merwok
merwok previously approved these changes Jan 21, 2021
@terryjreedy terryjreedy merged commit 644d528 into python:master Jan 22, 2021
@miss-islington
Copy link
Contributor

Thanks @verhovsky for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 22, 2021
Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
(cherry picked from commit 644d528)

Co-authored-by: Борис Верховский <boris.verk@gmail.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jan 22, 2021
@bedevere-bot
Copy link

GH-24295 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 22, 2021
Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
(cherry picked from commit 644d528)

Co-authored-by: Борис Верховский <boris.verk@gmail.com>
@bedevere-bot
Copy link

GH-24296 is a backport of this pull request to the 3.8 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Jan 22, 2021
miss-islington added a commit that referenced this pull request Jan 22, 2021
Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
(cherry picked from commit 644d528)

Co-authored-by: Борис Верховский <boris.verk@gmail.com>
@terryjreedy
Copy link
Member

Boris, you have 6 merged PRs before this. After checking them, I think you qualify for a listing in Misc/acks. If you make a PR with no issue or news to add yourself, with the improved English transliteration, and request my review, I will merge it, and ask for a 3.9 backport.

miss-islington added a commit that referenced this pull request Jan 22, 2021
Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
(cherry picked from commit 644d528)

Co-authored-by: Борис Верховский <boris.verk@gmail.com>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Co-authored-by: Éric Araujo <merwok@netwok.org>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants