Skip to content

Comments

fix: preserve tuple trailing comma (swev-id: sphinx-doc__sphinx-9367)#82

Open
casey-brooks wants to merge 1 commit intosphinx-doc__sphinx-9367from
noa/issue-80
Open

fix: preserve tuple trailing comma (swev-id: sphinx-doc__sphinx-9367)#82
casey-brooks wants to merge 1 commit intosphinx-doc__sphinx-9367from
noa/issue-80

Conversation

@casey-brooks
Copy link

Summary

  • add regression coverage for single- and nested single-element tuples in tests/test_pycode_ast.py
  • ensure _UnparseVisitor.visit_Tuple preserves trailing commas for single-element tuples

Reproduction

  1. export MAMBA_ROOT_PREFIX=/workspace/micromamba
  2. PYTHONPATH=/workspace/sphinx micromamba run -n sphinx-py39 pytest tests/test_pycode_ast.py -q

Observed Failure

FAILED tests/test_pycode_ast.py::test_unparse[(1,)-(1,)] - AssertionError: assert '(1)' == '(1,)'
FAILED tests/test_pycode_ast.py::test_unparse[((1,),)-((1,),)] - AssertionError: assert '((1))' == '((1,),)'

Testing

  • PYTHONPATH=/workspace/sphinx micromamba run -n sphinx-py39 pytest tests/test_pycode_ast.py -q

Fixes #80

@casey-brooks casey-brooks requested a review from a team December 26, 2025 19:16
@casey-brooks
Copy link
Author

Local validation:\n- PYTHONPATH=/workspace/sphinx micromamba run -n sphinx-py39 pytest tests/test_pycode_ast.py -q → 43 passed, 8 warnings\n- micromamba run -n sphinx-py39 flake8 sphinx/pycode/ast.py tests/test_pycode_ast.py → no issues

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Looks good—_UnparseVisitor.visit_Tuple now preserves trailing commas for single-element tuples and the added regression tests cover the previously failing cases. I wasn't able to run pytest locally (pytest not installed in this environment), but the new tests target the regression directly.

Copy link
Collaborator

@rowan-stein rowan-stein left a comment

Choose a reason for hiding this comment

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

LGTM. Verified spec compliance and tests cover regression.

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.

3 participants