Skip to content

bpo-20285: Improve help docs for object #4759

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 7 commits into from
May 24, 2019
Merged

Conversation

csabella
Copy link
Contributor

@csabella csabella commented Dec 9, 2017

Improve the docstring for object and show object methods when using help(object).

https://bugs.python.org/issue20285

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Wouldn't the beginners be confused by the term "superclass"?

@@ -873,7 +873,7 @@ def spilldata(msg, attrs, predicate):
thisclass = attrs[0][2]
attrs, inherited = _split_list(attrs, lambda t: t[2] is thisclass)

if thisclass is builtins.object:
if object is not builtins.object and thisclass is builtins.object:
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this change?

Copy link
Member

Choose a reason for hiding this comment

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

The have help(object) print the methods defined on object, the same as it does for every other class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One confusing thing about this module is that the argument that the docs are being created for is named is object. So, for this PR, object both can refer to the name of the argument and the class called object.

thisclass is the third value (index 2) in the sequence returned from inspect.classify_class_attrs(object), which is defined as "The class which defined this attribute" (from the docstring). When the value of the class is object, the current if statement if thisclass is builtins.object: results in that attribute being skipped from the output.

The change is to skip the logic when the argument value is not builtins.object so that the attributes will be displayed when help(object) is used. Without this change, all of the attributes are skipped because "The class which defined this attribute" is always object.

@@ -4663,6 +4663,8 @@ static PyMethodDef object_methods[] = {
{0}
};

PyDoc_STRVAR(object_doc,
"object()\n--\n\nThe superclass for all Python classes.");
Copy link
Contributor

Choose a reason for hiding this comment

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

for -> of

Copy link
Member

Choose a reason for hiding this comment

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

To me, 'of' has more of an implication of immediate superclass. But see my response to Serhiy.

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.

help(someclass) prints someclass.doc and then prints the methods defined 'here' (on someclass). The goal of the issue is to improve 'help(object)' in two ways.

  1. Replace the current docstring. I think my initial proposal needs at least one word added and further changes are up for discussion.

  2. Actually print the methods defined on object. The patch lists the same methods as returned by dir(object). The pydoc change looks correct to me. I presume that the format of the typeobject change is correct since Serhiy did not say otherwise.

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

@csabella
Copy link
Contributor Author

@terryjreedy I don't quite understand what changes you'd like me to make. I changed the docstring to match what you asked about on the bug tracker for #1, but I'm not sure what you mean by 'Actually print the methods defined on object.'. Thanks!

@terryjreedy
Copy link
Member

Your initial patch fixed the help output. The second has the wording I suggested on the issue. I will expand the news entry a bit.

@terryjreedy
Copy link
Member

Absent an even better suggestion, I would like to go with this docstring, resulting from pydev discussion (see the tracker).

"The base class of the class hierarchy.

When called, it accepts no arguments and returns a new featureless instance that has no instance attributes and cannot be given any."

@@ -4707,6 +4707,9 @@ static PyMethodDef object_methods[] = {
{0}
};

PyDoc_STRVAR(object_doc,
"object()\n--\n\nThe starting base class of all types and classes other\n"
Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with docstrings in C code, but it appears that 'classname()\n--\n\n' gets removed before making the rest into the class object doc attribute.

@@ -4707,6 +4707,9 @@ static PyMethodDef object_methods[] = {
{0}
};

PyDoc_STRVAR(object_doc,
"object()\n--\n\nThe starting base class of all types and classes other\n"
"than itself.");
Copy link
Member

Choose a reason for hiding this comment

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

The first line of a docstring, if not too long, should not have an embedded \n. So I think this should have been written

"object()\n--\n\n"
"The starting base class of all types and classes other than itself\n"

My suggested replacement would then be

"object()\n--\n\n"
"The base class of the class hierarchy.\n\n"
"When called, it accepts no arguments and returns a new featureless\n"
"instance that has no instance attributes and cannot be given any.\n"

A limit of 72 columns with a 4 column indent allows 68 chars. The longest above is 66.

@csabella
Copy link
Contributor Author

@terryjreedy I've applied the changes. Thanks!

@terryjreedy
Copy link
Member

help(object) now looks correct on my machine, after the update merge. Rather than run test suite locally, I pushed to have patch to core object tested with current code on all CI systems. I believe there have been other patches to object.c since last CI run.

@csabella
Copy link
Contributor Author

Thanks @terryjreedy! It looks like all the CI tests have passed, so I"m going to merge this.

@csabella csabella merged commit c95c93d into python:master May 24, 2019
@csabella csabella deleted the bpo20285 branch May 24, 2019 10:43
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants