Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-134733: fix: ast.dump( show_empty=True ) now displays allNone values#134743
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
Hmmm... It seems as if the ast tests are failing. The tests want |
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.
@hunterhogan The issue is that the tests haven't been updated to handle the kind="None" attr.
We could either drop that or update the tests.
I think I am missing something because the existing tests seem sparse. I made some tests, but I didn't finish integrating them. I don't really know what I am doing. I could use a little help, to be honest. |
| @@ -0,0 +1,1477 @@ | |||
| import ast | |||
| check_node( | |||
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.
What ischeck_node?
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.
From the existing tests.
cpython/Lib/test/test_ast/test_ast.py
Lines 1511 to 1539 infbbbc10
| deftest_dump_show_empty(self): | |
| defcheck_node(node,empty,full,**kwargs): | |
| withself.subTest(show_empty=False): | |
| self.assertEqual( | |
| ast.dump(node,show_empty=False,**kwargs), | |
| empty, | |
| ) | |
| withself.subTest(show_empty=True): | |
| self.assertEqual( | |
| ast.dump(node,show_empty=True,**kwargs), | |
| full, | |
| ) | |
| defcheck_text(code,empty,full,**kwargs): | |
| check_node(ast.parse(code),empty,full,**kwargs) | |
| check_node( | |
| ast.arguments(), | |
| empty="arguments()", | |
| full="arguments(posonlyargs=[], args=[], kwonlyargs=[], kw_defaults=[], defaults=[])", | |
| ) | |
| check_node( | |
| # Corner case: there are no real `Name` instances with `id=''`: | |
| ast.Name(id='',ctx=ast.Load()), | |
| empty="Name(id='', ctx=Load())", | |
| full="Name(id='', ctx=Load())", | |
| ) | |
sharktide commentedMay 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'll see what I can do. Busy now will get to this |
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.
I'm going file by file.
In the news entry we don't need the entire second sentence.
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.
@hunterhogan You never definedcheck_node.
I'm writing one now. Also, could you please add me as a collaborator to your branch so I can try a few things?
@hunterhogan test_ast/cases.py test_ast/utils.py test_ast/test_ast.py Is it okay if I do this? My version of check_node needs a specific format which would require rewriting all your tests |
I was attempting to integrate with the existing tests. cpython/Lib/test/test_ast/test_ast.py Lines 1511 to 1539 infbbbc10
I'm in over my head. |
I am not currently capable of accomplishing this task. #134718 will probably resolve it, though. |
@hunterhogan We'll see. Thanks for opening this PR and giving it a shot though! |
I'm going to try to revive this |
Please, ensure that tests are passed before creating a PR. I suspect also that the change proposed in this PR has flaws. Tests could expose them. |
I'll do my best |
I'm going to cherry-pick your code into a branch on my fork |
@serhiy-storchaka The existing tests didn't expose the obvious flaw in ast.dump. And the existing tests are erroneously failing when the function is doing what it is supposed to do. Are you willing to put some skin in the game? ast.dump may have flaws, but my changes didn't introduce them. US$500? That's ~20% of all of my money. |
@hunterhogan Please do not bet on github. |
Changes made
Baseless response
Calling out bad behavior
Community
I'm out |
Also please keep things civil. |
Thank you for quickly responding with false equivalence. It will help me have accurate expectations if I try to contribute again. |
@hunterhogan No offense, but seems like a bit of an overreaction. I am taking the time to write this because no one is denying that you are a bad programmer, but it is a bit 'uncivil' to get mad at people and bet money over PRs and Issues. Nothing personal, but please, there was nothing wrong with@serhiy-storchaka's comment IMO, so don't overreact if it just questioned this issue/PR. It is just called triaging and there was nothing against you. |
Based on the consistent tone in this PR I'm going to lock it down. |
Uh oh!
There was an error while loading.Please reload this page.
Ensure that ast.dump includes all None values when the show_empty flag is set to True.
Also, changed
iftoeliffor consistency.ast.dump( show_empty = True)does not show mostNonevalues. #134733