gh-134733: fix: ast.dump( show_empty=True ) now displays all None values

[hunterhogan]

Ensure that ast.dump includes all None values when the show_empty flag is set to True.

Also, changed if to elif for consistency.

• Issue: ast.dump( show_empty = True) does not show most None values. #134733

[sharktide]

Hmmm... It seems as if the ast tests are failing. The tests want res and expect to be the same and it isn't getting what it wants

[sharktide]

@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.

[hunterhogan]

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.

[serhiy-storchaka]

What is check_node?

[hunterhogan]

From the existing tests.

cpython/Lib/test/test_ast/test_ast.py

Lines 1511 to 1539 in fbbbc10

	def test_dump_show_empty(self):
	def check_node(node, empty, full, **kwargs):
	with self.subTest(show_empty=False):
	self.assertEqual(
	ast.dump(node, show_empty=False, **kwargs),
	empty,
	)
	with self.subTest(show_empty=True):
	self.assertEqual(
	ast.dump(node, show_empty=True, **kwargs),
	full,
	)
	def check_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]

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.

I'll see what I can do. Busy now will get to this

[sharktide]

I'm going file by file.

In the news entry we don't need the entire second sentence.

[sharktide]

@hunterhogan You never defined check_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?

[sharktide]

@hunterhogan
I got a working version of check_node, but I think we should implement this the following way:

test_ast/cases.py
<-- add test cases here in a list -->

test_ast/utils.py
<-- add my version of check_node here -->

test_ast/test_ast.py
<-- Call the cases with my check_node in utils -->

Is it okay if I do this? My version of check_node needs a specific format which would require rewriting all your tests

[hunterhogan]

I was attempting to integrate with the existing tests.

cpython/Lib/test/test_ast/test_ast.py

Lines 1511 to 1539 in fbbbc10

	def test_dump_show_empty(self):
	def check_node(node, empty, full, **kwargs):
	with self.subTest(show_empty=False):
	self.assertEqual(
	ast.dump(node, show_empty=False, **kwargs),
	empty,
	)
	with self.subTest(show_empty=True):
	self.assertEqual(
	ast.dump(node, show_empty=True, **kwargs),
	full,
	)
	def check_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())",
	)

I'm in over my head.

[hunterhogan]

I am not currently capable of accomplishing this task.

#134718 will probably resolve it, though.

[sharktide]

@hunterhogan We'll see. Thanks for opening this PR and giving it a shot though!

[sharktide]

I'm going to try to revive this

[serhiy-storchaka]

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.

[sharktide]

I'll do my best

[sharktide]

I'm going to cherry-pick your code into a branch on my fork

[hunterhogan]

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.

@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.

[sharktide]

@hunterhogan Please do not bet on github.

[hunterhogan]

Changes made

Baseless response

Calling out bad behavior

Community

I'm out

[brettcannon]

Also please keep things civil.

[hunterhogan]

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.

[sharktide]

@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.

[brettcannon]

Based on the consistent tone in this PR I'm going to lock it down.