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
gh-92788: Add docs for ast.Module, ast.Expression, and others
#101055
base: main
Are you sure you want to change the base?
Conversation
sobolevn
commented
Jan 15, 2023
•
edited by bedevere-bot
edited by bedevere-bot
- Issue: No documentation for ast.Module class and others. #92788
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-addeddirective.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-addeddirective 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-statementsref target label above theStatementssection, 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 <CAM.Gerlach@Gerlach.CAM>
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 <CAM.Gerlach@Gerlach.CAM>
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-addeddirective 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?