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-38524: add note about the implicit and explicit calling of __set_name__ #17364

Merged
merged 3 commits into from Nov 27, 2019

Conversation

@DahlitzFlorian
Copy link
Contributor

DahlitzFlorian commented Nov 23, 2019

As described in the corresponding issue a note about the implicit and explicit calling of __set_name__ should be added to the documentation.

@taleinat I cannot explicitly request your review. However, I mention you here as you said you'd be happy to review it. Let me know if I missed something.

https://bugs.python.org/issue38524

@DahlitzFlorian DahlitzFlorian changed the title bpo-38524: add note about the implicit/explicit calling of __set_name__ skefnefefefe bpo-38524: add note about the implicit and explicit calling of __set_name__ Nov 23, 2019
Copy link
Contributor

taleinat left a comment

Hi Florian, thanks for this PR!

IMO this section is not the appropriate place in the document to add such a note: This note is about adding descriptors to a class outside of the normal class creation process described in this section.

ISTM that "3.3.2.2. Implementing Descriptors" would be the best place for this. Including a reference to "3.3.3.6. Creating the class object" would certainly be appropriate.

@DahlitzFlorian

This comment has been minimized.

Copy link
Contributor Author

DahlitzFlorian commented Nov 24, 2019

Hi Tal, thanks for your reply!

I will have a closer look at it and adjust the files accordingly. I guess I should squash the commits together to end up with a single commit? A news entry is not needed here or am I wrong (cause of the failing bedevere/news check)?

@taleinat

This comment has been minimized.

Copy link
Contributor

taleinat commented Nov 24, 2019

Hi Florian,

Please don't change the history of commits in a branch with an open PR, i.e. don't rebase, squash etc. It makes understanding the history difficult.

We always squash & merge branches when merging, so there's no need for you to squash yourself :)

@DahlitzFlorian

This comment has been minimized.

Copy link
Contributor Author

DahlitzFlorian commented Nov 24, 2019

Okay, I've adjusted the files accordingly. Thanks for the hint with squash and rebase. Didn't find anything about it on devguide. I will have another look at it and if I can't find something like that I will submit issue and PR adding a note about it.

After reading the other sections I agree with you, that the note is better placed at the descriptors section.

@taleinat

This comment has been minimized.

Copy link
Contributor

taleinat commented Nov 24, 2019

Thanks for the hint with squash and rebase. Didn't find anything about it on devguide. I will have another look at it and if I can't find something like that I will submit issue and PR adding a note about it.

That would be great! I have worked on that recently as well so please do mention me if you make a PR about this.

@taleinat

This comment has been minimized.

Copy link
Contributor

taleinat commented Nov 24, 2019

From a bit more reading, it would seem also appropriate to suggest setting __objclass__ in similar cases as those where __set_name__ would need to be called explicitly. I'm not actually familiar with it and haven't the time right now to dig deeper, but it's worth checking out.

@DahlitzFlorian

This comment has been minimized.

Copy link
Contributor Author

DahlitzFlorian commented Nov 24, 2019

That would be great! I have worked on that recently as well so please do mention me if you make a PR about this.

This was already added in 2017 (see 3.8. Submitting last paragraph).

From a bit more reading, it would seem also appropriate to suggest setting __objclass__ in similar cases as those where __set_name__ would need to be called explicitly. I'm not actually familiar with it and haven't the time right now to dig deeper, but it's worth checking out.

In the upcoming days I will check that out, thanks for the hint! I will post my findings here on this PR. Is there anyone else we should mention to review the PR afterwards?

@taleinat

This comment has been minimized.

Copy link
Contributor

taleinat commented Nov 24, 2019

@DahlitzFlorian

Is there anyone else we should mention to review the PR afterwards?

That's not necessary, strictly speaking, as I'm a core dev. Also, this is a documentation PR, and not a major one at that. Let's see what comes up when we dig into __objattr__, and if we're unsure we can ping Serhiy and Nick who commented on the bpo issue.

@DahlitzFlorian

This comment has been minimized.

Copy link
Contributor Author

DahlitzFlorian commented Nov 25, 2019

From a bit more reading, it would seem also appropriate to suggest setting __objclass__ in similar cases as those where __set_name__ would need to be called explicitly. I'm not actually familiar with it and haven't the time right now to dig deeper, but it's worth checking out.

I only found one reference to __objclass__ in the documentation (right below the inserted note) and can't see why we should include a note about setting it, too. Perhaps someone else has more knowledge about it. The references inside the source code didn't enlighten me, either.

@taleinat

This comment has been minimized.

Copy link
Contributor

taleinat commented Nov 25, 2019

I only found one reference to __objclass__ in the documentation (right below the inserted note) and can't see why we should include a note about setting it, too.

From my initial reading, __objclass__ appears to be another thing one should explicitly set on descriptors added to a class after class initialization. It is expected to be there by certain tools, e.g. in the inspect and pydoc modules. If my understanding is correct, then it seems to make sense to mention it alongside __set_name__.

@python python deleted a comment Nov 25, 2019
@DahlitzFlorian

This comment has been minimized.

Copy link
Contributor Author

DahlitzFlorian commented Nov 26, 2019

As I already mentioned, I didn't find anything else about __objclass__ in the documentation than the paragraph after the note inserted by this PR. That's why I searched in the PEPs to find some more information about it and found it in PEP 252, PEP 575 and PEP 580. At least two of the helped me understand the purpose of __objclass__ better. However, I wasn't able to actually access __objclass__ anywhere. The paragraph below the note mentions, that the inspect module uses it. However, the inspect module on its own doesn't mention it in its documentation.

Manually inspecting the objects in the REPL didn't help, either. The example I worked with is listed below as descriptor.py. As I can't find an obvious usage, don't find anything about it in the corresponding documentations and wasn't able to access it in the REPL (deep deep down), I guess we don't need to mention it in the note. Nonetheless, I would be glad if someone else finds anything to support my thesis and merge the PR or correct it, so we can add it to the note and merge afterwards.

# descriptors.py
class Verbose_attribute():
    def __get__(self, obj, type=None) -> object:
        return obj.__dict__.get(self.name) or 0

    def __set__(self, obj, value) -> None:
        obj.__dict__[self.name] = value

    def __set_name__(self, owner, name):
        self.name = name


class Foo():
    attr = Verbose_attribute()


class Bar():
    pass


foo = Foo()
print(foo.__class__.__dict__["attr"].__dict__)

bar = Bar()

descr = Verbose_attribute()
Bar.attr = descr
print(bar.__class__.__dict__["attr"].__dict__)

descr.__set_name__(bar, "attr")
print(bar.__class__.__dict__["attr"].__dict__)
Copy link
Contributor

taleinat left a comment

LGTM

@taleinat

This comment has been minimized.

Copy link
Contributor

taleinat commented Nov 27, 2019

Further reading and experimenting hasn't brought up any indication of a need to set __objclass__, so we should indeed leave that alone. Thanks for digging into that @DahlitzFlorian!

@taleinat taleinat merged commit 1bddf89 into python:master Nov 27, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191125.11 succeeded
Details
bedevere/issue-number Issue number 38524 found
Details
bedevere/news "skip news" label found
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Nov 27, 2019

Thanks @DahlitzFlorian for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒🤖 I'm not a witch! I'm not a witch!

miss-islington added a commit to miss-islington/cpython that referenced this pull request Nov 27, 2019
…et_name__ (pythonGH-17364)

(cherry picked from commit 1bddf89)

Co-authored-by: Florian Dahlitz <f2dahlitz@freenet.de>
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 27, 2019

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

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 27, 2019

GH-17402 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 Nov 27, 2019
…et_name__ (pythonGH-17364)

(cherry picked from commit 1bddf89)

Co-authored-by: Florian Dahlitz <f2dahlitz@freenet.de>
@DahlitzFlorian

This comment has been minimized.

Copy link
Contributor Author

DahlitzFlorian commented Nov 27, 2019

Thanks @taleinat for guiding me!

@DahlitzFlorian DahlitzFlorian deleted the DahlitzFlorian:fix-issue-38524 branch Nov 27, 2019
miss-islington added a commit that referenced this pull request Nov 27, 2019
…et_name__ (GH-17364)

(cherry picked from commit 1bddf89)

Co-authored-by: Florian Dahlitz <f2dahlitz@freenet.de>
miss-islington added a commit that referenced this pull request Nov 27, 2019
…et_name__ (GH-17364)

(cherry picked from commit 1bddf89)

Co-authored-by: Florian Dahlitz <f2dahlitz@freenet.de>
jacobneiltaylor added a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.