Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-111746: Doc: Format built-in function's attributes#113574

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
adorilson wants to merge13 commits intopython:main
base:main
Choose a base branch
Loading
fromadorilson:gh111746_fix_list_attr_datamodel_doc

Conversation

adorilson
Copy link
Contributor

@adorilsonadorilson commentedDec 29, 2023
edited by hugovk
Loading

adorilsonand others added5 commitsDecember 27, 2023 18:20
The check about the f argument type was removed in this commit:python@2c94aa5Thanks for Pedro Arthur Duarte (pedroarthur.jedi at gmail.com) by the help withthis bug.
…#106335)Remove private _PyThreadState and _PyInterpreterState C APIfunctions: move them to the internal C API (pycore_pystate.h andpycore_interp.h). Don't export most of these functions anymore, butstill export functions used by tests.Remove _PyThreadState_Prealloc() and _PyThreadState_Init() from the CAPI, but keep it in the stable API.
Copy link
Member

@hugovkhugovk left a comment

Choose a reason for hiding this comment

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

Looks good, one comment.

@hugovkhugovk added needs backport to 3.11only security fixes needs backport to 3.12only security fixes labelsDec 30, 2023
@@ -666,9 +666,14 @@ callable object (normally a user-defined function).
single: __name__ (method attribute)
single: __module__ (method attribute)

Special read-only attributes:
Special read-only attributes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it really makes sense to make this a separate subsection, since there are no other subsections inside the datamodel section on instance methods. I'd just keep this as a sentence ending with a colon, personally

the module the function was defined in or ``None`` if unavailable.
See :attr:`function.__module__`.
Special read-only attributes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I'm not sure it really makes sense to make this a distinct subsection when there aren't any other subsections inside the datamodel section on builtin functions

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I thought about that, and I decided by subsection because:

  1. consistency with other sections (e.g., the one you highlighted above and Code Objects). In this case, I would fix some other sections, too.
  2. personally, I like that (although it is a unique section).
  3. if I see the summary, I immediately know the element has or does not have special attributes. More explicit.

Actually, when I opened the issue, I thought about a unique table for both attributes with a column to identify the kind, as theUser-defined functions section was before your recent changes.

Copy link
Member

Choose a reason for hiding this comment

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

Those are all reasonable points! I don't mind the new subsection in the section on builtin functions too much; I'm happy for that to stay.

Ido think the new subsection in the section on instance methods makes it more confusing, however. It implies that the entirety of the section on instance methods is about their special read-only attributes, except for the first paragraph. This isn't true: the final paragraph in section 3.2.8.2 has nothing to do with the special read-only methods of instance methods:

Note that the transformation fromfunction object to instance method object happens each time the attribute is retrieved from the instance. In some cases, a fruitful optimization is to assign the attribute to a local variable and call that local variable. Also notice that this transformation only happens for user-defined functions; other callable objects (and all non-callable objects) are retrieved without transformation. It is also important to note that user-defined functions which are attributes of a class instance are not converted to bound methods; this only happens when the function is an attribute of the class.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

What if do we move this paragraph to the second paragraph? This seems an explanation for the first one.

Copy link
Member

Choose a reason for hiding this comment

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

Well, what about these two paragraphs?

When an instance method object is created by retrieving aclassmethod object from a class or instance, its__self__ attribute is the class itself, and its__func__ attribute is the function object underlying the class method.

When an instance method object is called, the underlying function (__func__) is called, inserting the class instance (__self__) in front of the argument list. For instance, when C is a class which contains a definition for a function f(), and x is an instance of C, calling x.f(1) is equivalent to calling C.f(x, 1).

The first paragraph is about the special read-only attributes of method objects; the second one isn't (it's about a broader topic to do with the semantics of Python methods). So maybe we should move the second paragraph out of the section about the special read-only attributes -- but that would make no sense; it's much more logical to talk about how theself reference is inserted at the beginning of the argument list after we've talked about the special__self__ attribute. The two paragraphs belong together.

For the section on instance methods, I think the existing structure -- where the special read-only attributes are discussedat the same time as other details on method semantics -- makes most sense. Unless you want to propose reworking the whole section on instance methods (which should go into a separate PR, IMO), I don't think we should add a subsection just for the read-only attributes.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

it's about a broader topic to do with the semantics of Python methods

Despite this, I think that we can consider all these paragraphs as about the attributes. They talk about how the attributes work together to element (object, class, method..) to work as well. The attributes are cited a lot of time.

In first look, I understand this paragraph under attributes this way. However, I also noted some paragraphs didn't are about the attribute, but about the element. I would propose moving these paragraphs to the introduction of the section, probrably.

Another two idea are:

  1. Adding a new section something like "More details", "How it works" about extra content.
  2. Removingall attribute's subsection mark in the chapter

I prefer first option (although more work), but the second one also provide consistence.

Unless you want to propose reworking the whole section on instance methods (which should go into a separate PR, IMO),

Shall we do this together. Some details about it can scape me. :) I think what I told above is in this direction.

(Sorry for any confused thought. It is a brainstorm)

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, but I still disagree that using a subsection for theInstance methods section is the right way to go there, at least the way you've currently done it in this PR :/ I'd be happy to look at a broader rewrite of that section, because I agree that consistency between the different sections is nice. But unfortunately I don't really have time to push forward on that right now (and I doubt I will until PyCon).

Comment on lines +823 to +825
* - .. attribute:: builtin_function.__class__
- The function's class.

Copy link
Member

Choose a reason for hiding this comment

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

This attribute is present on all Python objects everywhere, and is already documented as such here:https://docs.python.org/3/library/stdtypes.html#special-attributes. Please revert this addition; there's no need to duplicate that documentation here.

Suggested change
* - .. attribute:: builtin_function.__class__
- The function's class.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

In fact, I've realized that attribute definition duplication is a problem in this chapter. See__doc__ and__name__ attributes, e.g.

I'm thinking about a kind of hierarchy: a section with all common attributes and in each callable type something like "the attributes are those above plus these more: ... ". The content that you highlighted now is a good beginning in this direction. However, Reference Language must be referenced by the Library Reference. Besides, we don't have a reference to the cited content in this chapter.

Also, I found theattribute definitions in theinspect module documentation. This could be erased and added a refer to the Language Reference.

In my opinion, the Language Reference is more important than other sections. All other sections must refer to the Reference, not the contrary.

So, my suggestion is:

  1. Keeping this definition (although the duplication). This helps make explicit the problem.
  2. Continue this review in this chapter
  3. Discussing a format to avoid duplication inside this chapter (and implement it)
  4. Removing duplication outside this chapter (the section you cited andinspect, for example) and refer to it

What do you think? Any other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I've been working towards similar goals to you in my recent PRs#112781,#112933,#112735, etc. My opinions:

  • For attributes that really do exist on all objects everywhere, like__class__ and__dict__, the list instdtypes.rst is useful. We should keep the list for things like that. Maybe the list should be moved out ofstdtypes.rst and into the datamodel somewhere, but that's a separate question from whether we should keep the list or not, and I think we should keep the list.
  • For attributes that don't exist on all objects everywhere, like__name__ and__doc__, we should document them on the specific objects for which they exist in the datamodel, and eventually delete them from the list instdtypes.rst. I've been working towards that in my recent PRs.
  • Once all things in the big table ininspect.rst are properly documented, we should delete it frominspect.rst and point to the datamodel docs. I have it on my to-do list to file PRs to improve documentation for attributes on class objects, module objects and coroutine objects.

adorilson reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

  • For attributes that really do exist on all objects everywhere, like__class__ and__dict__, the list instdtypes.rst is useful. We should keep the list for things like that. Maybe the list should be moved out ofstdtypes.rst and into the datamodel somewhere, but that's a separate question from whether we should keep the list or not, and I think we should keep the list.

My suggestion is to put the list onThe standard type hierarchy. The second paragraph already cited special attributes, so this seems like a good place to show them.

Does it make sense for you?

Copy link
Member

@AlexWaygoodAlexWaygoodJan 3, 2025
edited
Loading

Choose a reason for hiding this comment

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

Some things have changed since we had this conversation in March!

Now,__class__ and__dict__ are documented asobject.__class__ andobject.__dict__, because they are present onall Python objects:https://docs.python.org/3/reference/datamodel.html#object.__class__,https://docs.python.org/3/reference/datamodel.html#object.__dict__. But__name__ and__doc__ are now more consistently listed for classes documented in the data model that define these attributes, and they are still listed inhttps://docs.python.org/3/library/stdtypes.html#special-attributes (because they do not exist onall Python objects).

All things considered, I still think it's incorrect for the__class__ attribute to be documented here, since it doesn't really add anything to the documentation athttps://docs.python.org/3/reference/datamodel.html#object.__class__. So my original request remains: please get rid of this bullet point :-)

My suggestion is to put the list onThe standard type hierarchy. The second paragraph already cited special attributes, so this seems like a good place to show them.

Let's have this conversation in an issue or PR elsewhere; the question doesn't really seem relevant to this PR :-) Maybe the list should be moved out of stdtypes into the datamodel, and maybe not -- whatever the case, I don't think it should be done as part of this PR :-)

hugovk reacted with thumbs up emoji
@bedevere-app
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 phraseI 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.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@adorilson
Copy link
ContributorAuthor

I have made the requested changes; please review again

@adorilson
Copy link
ContributorAuthor

@AlexWaygood

I've committed the__self__ change. However, look at#58211.

@AlexWaygoodAlexWaygood removed their request for reviewDecember 31, 2023 17:39
@AlexWaygood
Copy link
Member

I've committed the__self__ change. However, look at#58211.

Oh, interesting. Hmm, not sure what we should do here. It seems like a misfeature, at best, that__self__ is set to module objects for builtin functions -- so maybe we shouldn't document that. Maybe the best course of action would be to say something a bit more vague like "While this may exist as an attribute on builtin function objects, its value is never meaningful (but see [section on builtin methods])".@hugovk any thoughts?

@adorilson
Copy link
ContributorAuthor

. Maybe the best course of action would be to say something a bit more vague like "While this may exist as an attribute on builtin function objects, its value is never meaningful (but see [section on builtin methods])"

In this PR, I'm unsure because the module object (the currently__self__ value) is significant.

This discussion must be on the issue that I cited. I proposed PR#113575 with the test to the__self__, and I'm waiting for this PR to be merged to include the doc in that PR. In the worst scenario, the discussion about that will be reopened.

So, two ideas:

  1. Keeping the text with the suggestion in this PR
  2. Adding a (foot)note highlighting the bug. Avoid some confusion and make it clear and transparent.

@hugovkhugovk removed the needs backport to 3.11only security fixes labelApr 11, 2024
@serhiy-storchakaserhiy-storchaka added the needs backport to 3.13bugs and security fixes labelMay 9, 2024
@adorilson
Copy link
ContributorAuthor

@hugovk, do you have any direction here?

I'd like to make a decision at this point and then apply it to the next document sections.

@AlexWaygood
Copy link
Member

AlexWaygood commentedJan 3, 2025
edited
Loading

The update on the conversation regarding__self__ is that it looks like#113575 might be accepted, but it's still awaiting changes that Serhiy asked for (and which I agree with)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@AlexWaygoodAlexWaygoodAlexWaygood requested changes

@hugovkhugovkAwaiting requested review from hugovk

@willingcwillingcAwaiting requested review from willingcwillingc is a code owner

Assignees
No one assigned
Labels
awaiting changesdocsDocumentation in the Doc dirneeds backport to 3.13bugs and security fixesneeds backport to 3.14bugs and security fixesskip news
Projects
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@adorilson@AlexWaygood@hugovk@serhiy-storchaka@vstinner

[8]ページ先頭

©2009-2025 Movatter.jp