Skip to content

bpo-20751: Replace method example with attribute example, matching the descriptor howto guide #29909

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 25 commits into from
Dec 4, 2021

Conversation

rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Dec 3, 2021

rhettinger and others added 24 commits March 15, 2021 21:12
.
Merge branch 'master' of github.com:python/cpython
.
Merge branch 'master' of github.com:python/cpython
.
Merge branch 'master' of github.com:python/cpython
.
Merge branch 'master' of github.com:python/cpython
Merge branch 'main' of github.com:python/cpython into main
Merge branch 'main' of github.com:python/cpython
@rhettinger rhettinger merged commit 135ecc3 into python:main Dec 4, 2021
@miss-islington
Copy link
Contributor

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @rhettinger, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 135ecc3492cee259090fd4aaed9056c130cd2eba 3.10

@rhettinger rhettinger added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Dec 4, 2021
@miss-islington
Copy link
Contributor

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @rhettinger, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 135ecc3492cee259090fd4aaed9056c130cd2eba 3.10

Arthur-Milchior added a commit to Arthur-Milchior/cpython that referenced this pull request Dec 26, 2021
I fear that the section "invoking descriptor" was quite hard to grasp on first
reading. I try to improve this with the following changes.

The notion of lookup chain was used without ever being defined anywhere in the
documentation. I add a short definition.

> may override the default behavior and invoke the descriptor method instead

was changed. It seemed to indicate that descriptor replace the behavior
described previously, of checking for the value associated to `x` in `a` in the
lookup chain. To the best of my understanding, the lookup chain is still used,
since we do not even know whether `x` is a descriptor or not until the lookup
chain returned a value. It would be more exact to indicate that a descriptor
override the action of returning, assigning or deleting.

> The starting point for descriptor invocation is a binding, ``a.x``. How the
> arguments are assembled depends on ``a``:

While python#29909 was a net improvement, I fear it was not sufficient. I had three
issues while reading this section for a first time.
* The Direct Call actually do not use the binding `a.x` (that's the very point
of the direct call), so the Direct Call should not have been in this
enumeration. In fact, what is even worse is that in the Direct Call, `x` is a
variable while in all other case, `x` is an attribute name.
* Furthermore, once I was reading the Super Binding, it was confusing that what
was called `a` in the introduction of the list is now called `super(A, a)`. At
this point, I believe it is more clear to just state that there are four ways to
invoke a descriptor and to list them. Each case introducing the variables it
requires.pp
* Super Binding uses the name "dotted lookup" while the remaining of the
documentation uses "attribute access". This lead me to assume that there is a
reason why a different name was used, that it was referencing a different
concept. I believe it is best to be consistent with naming of a concept.
* Super Binding "for a base class ``B`` following ``A``" was slightly confusing,
as it seems to indicates that there may exists multiple such base class `B`. I
believe that "the base class ``B``" is clearer.

For your information "dotted lookup" is also used in the howto section; I'd
suggest to change it there too for the sake of consistency. But since the usage
is consistent throughout the howto's descriptor file, this is at least less
confusing than this usage.
@ZeroIntensity ZeroIntensity removed the needs backport to 3.10 only security fixes label Feb 17, 2025
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 skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants