-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-92788: Add docs for ast.Module
, ast.Expression
, and others
#101055
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Higher-level comments:
-
Since this is formally documenting new module classes, IMO it would probably be worth adding a NEWS entry under the Documentation category (and perhaps even a What's New entry?), and perhaps also adding a
version-added
directive.Alternatively, if this is considered fixing a documentation defect to properly document nodes that always were de-facto considered public, and thus doesn't deserve a What's New or NEWS entry, then it might be worth backporting to earlier versions, assuming they also have the same nodes (and if they don't, then an appropriate
version-added
directive should be added accordingly). -
You should at least briefly but explictly document the parameters to each class constructor (type and value), as my comment does for body and as the other node classes generally do.
-
You'll need to add a
ast-statements
ref target label above theStatements
section, i.e... _ast-statements: Statements ^^^^^^^^^^
Wow, what a great feedback! 👍
Yeah, I think that this is the case. These nodes are public since day one, they are also supported by python3.6 and earlier (except |
Co-authored-by: C.A.M. Gerlach <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, great work—thanks @sobolevn ! I've left some additional minor comments as suggestions; otherwise I think this is ready for core dev/subject matter expert review (@isidentical ?)
Co-authored-by: C.A.M. Gerlach <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now; there's just one remaining top-level comment from the previous review.
Since this is formally documenting stdlib module classes, IMO it would make sense to do one of the following:
-
If it's considered to be formally documenting these classes for the first time, and they are not considered public in previous Python versions, then it seems certainly worth adding a NEWS entry under the Documentation category (and perhaps even a What's New entry?), and perhaps also adding a version-added directive.
-
Alternatively, if this is considered fixing a documentation defect to properly document nodes that were already de-facto considered public, and thus doesn't deserve a What's New entry or
versionchanged
, then it would presumably be worth backporting to earlier versions, assuming they also have the same nodes (and if they don't, then an appropriateversion-added
directive should be added accordingly). It might still be worth a Documentation-category NEWS entry, though.
From a Docs team perspective, I would prefer the second option, barring a strong enough reason, but I'm not the subject matter expert on ast
here. @isidentical , what do you think?
I've been working a lot with the However, it's still lacking an explanation of how to use the |
@ajoino yes, I think this is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an explicit mention to each class description of it's connection with the ast.parse
mode
, per @isidentical 's suggestions. Also, added cross-references to the relevant section for each in the Top-Level Components page of the Language Reference, for readers interested in the technical details (as found through the discussion above viz. interactive input).
Co-authored-by: C.A.M. Gerlach <[email protected]>
@CAM-Gerlach @isidentical done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now from my end 👍
@pablogsal @lysnikolaou maybe you would have time to take a look? :) |
Thanks @sobolevn for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Thanks @sobolevn for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
GH-106138 is a backport of this pull request to the 3.11 branch. |
GH-106139 is a backport of this pull request to the 3.12 branch. |
…rs (pythonGH-101055) (cherry picked from commit 33608fd) Co-authored-by: Nikita Sobolev <[email protected]>
…rs (pythonGH-101055) (cherry picked from commit 33608fd) Co-authored-by: Nikita Sobolev <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.