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

gh-92788: Add docs for ast.Module, ast.Expression, and others #101055

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 15, 2023

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a 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 the Statements section, i.e.

    .. _ast-statements:
    
    Statements
    ^^^^^^^^^^

Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member Author

sobolevn commented Jan 16, 2023

Wow, what a great feedback! 👍
Thank you! I've applied one of your proposed changes, so you also get a credit for this PR (it was easier for me to fix others in a proper IDE, than here).

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

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 FunctionType which was added in python3.8)

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a 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 ?)

Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Doc/library/ast.rst Outdated Show resolved Hide resolved
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a 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 appropriate version-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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants