Skip to content

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

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

Merged
merged 14 commits into from
Apr 24, 2024
Merged

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 28, 2024

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/

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.

Thank you for picking this up!

@sobolevn sobolevn requested a review from JelleZijlstra March 14, 2024 08:48
@sobolevn
Copy link
Member Author

Updated! Thank you!

@sobolevn
Copy link
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
Member Author

Maybe @carljm or @hauntsaninja ? :)

Copy link
Contributor

@hauntsaninja hauntsaninja 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 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
Copy link
Member

carljm commented Mar 21, 2024

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
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
@sobolevn
Copy link
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!

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

LGTM, modulo comments. Thanks!

Copy link
Member

@carljm carljm left a comment

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.

@sobolevn
Copy link
Member Author

(Waiting for one more day)

@sobolevn sobolevn merged commit 692e902 into python:main Apr 24, 2024
33 checks passed
@sobolevn
Copy link
Member Author

Thanks everyone! 🎉

@sobolevn
Copy link
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