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.

Sign up for GitHub

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

Jump to bottom

gh-64019: Add missing module attribute in inspect table #98116

Merged
merged 4 commits into from
Nov 25, 2022
Merged

gh-64019: Add missing module attribute in inspect table #98116

merged 4 commits into from
Nov 25, 2022

Conversation

slateny
Copy link
Contributor

@slateny slateny commented Oct 9, 2022 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Oct 9, 2022
@slateny slateny mentioned this pull request Oct 9, 2022
Closed
JelleZijlstra
JelleZijlstra approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

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

@ericsnowcurrently would you like to take a look since you reviewed the previous version of this PR?

ericsnowcurrently
ericsnowcurrently approved these changes Oct 10, 2022
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

However, before merging, I recommend you get the opinion of @warsaw and @brettcannon; they are currently working on removing some of these attributes.

AA-Turner
AA-Turner approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

I think #65961 is the correct issue for the deprecations/removals.

A

@AA-Turner AA-Turner added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Oct 11, 2022
@brettcannon
Copy link
Member

I personally think the table shouldn't exist in the first place. The common attributes for objects is not inspect-specific, and so this is an odd place to document such things. It also leads to it being out-of-date when folks don't even know about it. So my preference it to instead remove the entire table.

Barring that, __cached__, __package__, and __loader__ should not be documented as they are deprecated in Python 3.12.

@slateny
Copy link
Contributor Author

slateny commented Oct 15, 2022

I see, what about removing the table section on modules and adding a sentence above it linking to https://docs.python.org/dev/reference/import.html#import-related-module-attributes? It would preserve the info in this page if needed and avoid having two sources of information.

@brettcannon
Copy link
Member

see, what about removing the table section on modules and adding a sentence above it linking

Works for me!

ericsnowcurrently
ericsnowcurrently approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

FWIW, it might be more clear to move the new sentence (parenthetical reference) to below the table, but I'll leave that to your (or others') discretion.

@slateny
Copy link
Contributor Author

slateny commented Oct 19, 2022

I considered putting it at the bottom, but since the module was at the top of the table I didn't want to move it too far away. But it does look a bit awkward, especially if more sections if the table will be removed, so if others feel the same I have no problem with moving it down.

@brettcannon brettcannon self-requested a review October 20, 2022 19:07
@smontanaro
Copy link
Contributor

(I'm working my way through some PRs which have been approved and are labeled "awaiting merge", hence my seemingly bolt from the blue comment. Why? Read here.)

This has been idle for awhile. It's been approved by @ericsnowcurrently and @JelleZijlstra. @brettcannon is there any reason not to merge?

@brettcannon
Copy link
Member

is there any reason not to merge?

Nope, just not at the top of my review queue. 😅

@brettcannon brettcannon merged commit 7d2dcc5 into python:main Nov 25, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @slateny and @brettcannon, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 7d2dcc53d09fe903329926bf7bbfe460b1465dab 3.11

@bedevere-bot
Copy link

GH-99782 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2022
@slateny @MichaelAnckaert @miss-islington
pythongh-64019: Have attribute table in inspect docs link to module…
bfc24f9 
… attributes instead of listing them (pythonGH-98116)

(cherry picked from commit 7d2dcc5)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Co-authored-by: Michael Anckaert <michael.anckaert@sinax.be>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Nov 25, 2022
@brettcannon brettcannon mentioned this pull request Nov 25, 2022
Closed
@brettcannon
Copy link
Member

I had trouble checking out the 3.11 backport branch.

Due to the backport problem I am going to close the 3.10 branch to make backporting easier. But I'm also not going to worry about backporting to 3.11 since this is a minor clean-up. If someone wants to create a PR for 3.11 I'm willing to review, but I have too much other PRs to review to do the backport myself.

@AlexWaygood AlexWaygood added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Nov 25, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-99783 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Nov 25, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2022
@slateny @MichaelAnckaert @miss-islington
pythongh-64019: Have attribute table in inspect docs link to module…
8aac1ef 
… attributes instead of listing them (pythonGH-98116)

(cherry picked from commit 7d2dcc5)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Co-authored-by: Michael Anckaert <michael.anckaert@sinax.be>
@AlexWaygood AlexWaygood added the needs backport to 3.10 only security fixes label Nov 25, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 25, 2022 •
edited
Loading

I had trouble checking out the 3.11 backport branch.

Due to the backport problem I am going to close the 3.10 branch to make backporting easier. But I'm also not going to worry about backporting to 3.11 since this is a minor clean-up. If someone wants to create a PR for 3.11 I'm willing to review, but I have too much other PRs to review to do the backport myself.

Removing and re-adding the label fixed the problem! (It usually does, whenever miss-islington says "I had trouble checking out the branch" rather than "I couldn't do the backport, there was a merge conflict".)

@AlexWaygood AlexWaygood added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes labels Nov 25, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington added a commit that referenced this pull request Nov 25, 2022
@miss-islington @slateny @MichaelAnckaert
gh-64019: Have attribute table in inspect docs link to module attri…
f5fa17b 
…butes instead of listing them (GH-98116)

(cherry picked from commit 7d2dcc5)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Co-authored-by: Michael Anckaert <michael.anckaert@sinax.be>
@ericsnowcurrently ericsnowcurrently mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettcannon brettcannon brettcannon approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@AA-Turner AA-Turner AA-Turner approved these changes

Assignees

@brettcannon brettcannon

Labels
docs Documentation in the Doc dir needs backport to 3.10 only security fixes skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@slateny @brettcannon @smontanaro @miss-islington @bedevere-bot @AlexWaygood @JelleZijlstra @ericsnowcurrently @AA-Turner @MichaelAnckaert