Skip to content

gh-116023: Add show_empty=False to ast.dump#116037

Merged
sobolevn merged 14 commits into
python:mainfrom
sobolevn:issue-116023
Apr 24, 2024
Merged

gh-116023: Add show_empty=False to ast.dump#116037
sobolevn merged 14 commits into
python:mainfrom
sobolevn:issue-116023

Conversation

@sobolevn

@sobolevn sobolevn commented Feb 28, 2024

Copy link
Copy Markdown
Member

My thoughts:

  • This parameter should be optional and switched off by default, so we preserve the output if people rely on it: as debugging tool or somewhere in tests
  • It should not touch attributes, because they already have a different flag

📚 Documentation preview 📚: https://cpython-previews--116037.org.readthedocs.build/

Comment thread Doc/library/ast.rst Outdated

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for picking this up!

Comment thread Lib/ast.py Outdated
Comment thread Lib/ast.py Outdated
Comment thread Lib/ast.py Outdated
Comment thread Lib/test/test_ast.py
@sobolevn sobolevn requested a review from JelleZijlstra March 14, 2024 08:48
Comment thread Doc/library/ast.rst Outdated
Comment thread Lib/ast.py Outdated
Comment thread Lib/ast.py Outdated
@sobolevn

Copy link
Copy Markdown
Member Author

Updated! Thank you!

@sobolevn

Copy link
Copy Markdown
Member Author

I will wait for some third opinion, maybe someone else has input: whether this should be a default or not.

Thanks for the review!

@sobolevn

Copy link
Copy Markdown
Member Author

Maybe @carljm or @hauntsaninja ? :)

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think if we have the feature it should be on by default. Since it's sort of a convenience / conciseness feature, it's not as useful if it's off by default. Also seems fairly safe to keep on by default, since that's basically what 3.12's ast.dump will give you.

@carljm

carljm commented Mar 21, 2024

Copy link
Copy Markdown
Member

FWIW, I agree with @hauntsaninja and @JelleZijlstra. I think in the more common use cases, seeing empty values is noisy and not useful, and show_empty=True can be passed if you want to see them. I don't think there are any guarantees around backwards-compatibility for the textual representation of an AST.

that's basically what 3.12's ast.dump will give you.

That depends on whether you are talking about ast.dump(ast.arguments()) (dumping a manually-constructed AST) or ast.dump(ast.parse("def x(): pass")) (dumping a parsed AST). For the latter case, show_empty=False is a new behavior that no previous Python version has ever had.

I still think it's the better behavior, though. I would rather see

Module(body=[FunctionDef(name='x', args=arguments(), body=[Pass()])])

by default, instead of

Module(body=[FunctionDef(name='x', args=arguments(posonlyargs=[], args=[], kwonlyargs=[], kw_defaults=[], defaults=[]), body=[Pass()], decorator_list=[], type_params=[])], type_ignores=[]).

@sobolevn sobolevn changed the title gh-116023: Add show_empty=True to ast.dump gh-116023: Add show_empty=False to ast.dump Mar 22, 2024
@sobolevn

Copy link
Copy Markdown
Member Author

Ok, fair enough! I've changed the default from show_empty=True to show_empty=False.
I've also updated tests and docs. Please, take a look at my new changes :)

@sobolevn sobolevn requested a review from hauntsaninja April 12, 2024 04:45
@sobolevn sobolevn requested a review from JelleZijlstra April 12, 2024 04:45
Comment thread Lib/test/test_ast.py
Comment thread Lib/test/test_ast.py
@sobolevn

Copy link
Copy Markdown
Member Author

Waiting for some time for @hauntsaninja and @carljm to provide feedback :)
Jelle, thanks a lot for the review and for finding annotate_fields bug!

@carljm carljm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, modulo comments. Thanks!

Comment thread Doc/library/ast.rst Outdated
Comment thread Lib/ast.py Outdated
Comment thread Misc/NEWS.d/next/Library/2024-02-28-11-51-51.gh-issue-116023.CGYhFh.rst Outdated

@carljm carljm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, one minor nit on the docs.

Comment thread Doc/library/ast.rst Outdated
Co-authored-by: Carl Meyer <carl@oddbird.net>
@sobolevn

Copy link
Copy Markdown
Member Author

(Waiting for one more day)

@sobolevn sobolevn merged commit 692e902 into python:main Apr 24, 2024
@sobolevn

Copy link
Copy Markdown
Member Author

Thanks everyone! 🎉

@sobolevn

Copy link
Copy Markdown
Member Author

Maybe we should also mention this in 3.13.rst in ast section?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants