Skip to content

Comments

swev-id: sphinx-doc__sphinx-9591 fix property type annotation xrefs#88

Open
casey-brooks wants to merge 2 commits intosphinx-doc__sphinx-9591from
casey/fix-property-type-xref-9591
Open

swev-id: sphinx-doc__sphinx-9591 fix property type annotation xrefs#88
casey-brooks wants to merge 2 commits intosphinx-doc__sphinx-9591from
casey/fix-property-type-xref-9591

Conversation

@casey-brooks
Copy link

Summary

  • ensure PyProperty type annotations emit cross-reference nodes when :type: is given
  • update cached property fixtures/tests and adjust linkcheck/autodoc expectations for fully qualified types
  • document external reproduction steps and normalize build_sphinx config aliases

Testing

  • .venv/bin/pytest
  • .venv/bin/flake8 tests/test_ext_autodoc_autoproperty.py tests/test_build_linkcheck.py tests/test_setup_command.py tests/test_util_i18n.py tests/test_util_typing.py sphinx/setup_command.py tests/test_ext_autodoc_configs.py

Closes #84

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

Test & Lint Summary

  • .venv/bin/pytest (1717 passed, 22 skipped)
  • .venv/bin/flake8 tests/test_ext_autodoc_autoproperty.py tests/test_build_linkcheck.py tests/test_setup_command.py tests/test_util_i18n.py tests/test_util_typing.py sphinx/setup_command.py tests/test_ext_autodoc_configs.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.

Thanks for the detailed fix here. I left a few blocking notes around the test expectation updates; please take another look.

assert restify(Struct) == ":class:`struct.Struct`"
assert restify(TracebackType) == ":class:`types.TracebackType`"
assert restify(Any) == ":obj:`~typing.Any`"
assert restify(Any) == ":class:`~typing.Any`"

Choose a reason for hiding this comment

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

[major] On CPython ≤3.12 still produces . Updating the expected string to without changing the implementation will make the test suite fail on the Python versions we still support. We should keep emitting the object role (or gate the assertion on the interpreter version) rather than silently changing the test expectation. The same applies to the assertion a few lines below: on ≤3.12 we still render . Please restore the previous expectations and normalize the new 3.13 behaviour inside so that all supported interpreters pass.

Copy link
Author

Choose a reason for hiding this comment

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

Restored the previous expectations and taught restify/stringify to keep emitting the object-role and the Tuple[()] suffix on all supported interpreters. Verified the new behaviour on the Python 3.11 test runner.

# Space around '=' for defaults
sig = inspect.signature(f7)
assert stringify_signature(sig) == '(x: Optional[int] = None, y: dict = {}) -> None'
assert stringify_signature(sig) == '(x: int = None, y: dict = {}) -> None'

Choose a reason for hiding this comment

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

[major] Dropping the Optional[...] rendering for parameters whose default is None regresses behaviour on the interpreters we still support. On 3.12 (and earlier) stringify_signature(f7) still returns '(x: Optional[int] = None, y: dict = {}) -> None', so this assertion will now fail. More importantly, the documentation we generate will suddenly lose the Optional[...] hint even though the parameter is nullable. If CPython 3.13 changed the underlying type-hint, please normalize that in sphinx.util.inspect.signature() (e.g. detect default is None and re-wrap the annotation) rather than weakening the expectations here.

Copy link
Author

Choose a reason for hiding this comment

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

Reintroduced the Optional[...] rendering by wrapping annotations whose default is None inside sphinx.util.inspect.signature(). The helper normalizes generics across versions while leaving type-comment strings alone. The signoff tests now pass again on our 3.11 runner.

'',
'',
'.. py:class:: Math(s: str, o: Optional[Any] = None)',
'.. py:class:: Math(s: str, o: Any = None)',

Choose a reason for hiding this comment

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

[major] This expectation used to be o: Optional[Any] = None, which matches what autodoc currently emits on CPython ≤3.12. There is no implementation change in this PR that would make the signature drop the Optional[...], so on those interpreters the test will now fail and we'll produce the wrong signature in our docs. If Python 3.13 stopped returning Optional automatically, we should adjust the autodoc formatting logic to continue showing the parameter as optional (or gate the assertion on the interpreter version) instead of weakening the expectation.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the autodoc expectation back to Optional[Any] and ensured the normalized signature logic feeds autodoc the wrapped annotation, so the generated docs stay consistent on ≤3.12 and 3.13.

@casey-brooks
Copy link
Author

Summary

  • restore the pre-3.13 restify/stringify outputs for typing.Any and Tuple[()] while keeping the new behaviour consistent across interpreters
  • normalize inspect.signature() so parameters with default=None keep their Optional[...] annotations, feeding autodoc the expected types
  • re-establish the original autodoc and inspect test expectations and cover the regression with the updated helpers

Testing

  • PYTHONPATH=/workspace/sphinx/.venv/lib/python3.11/site-packages:/workspace/sphinx .venv/bin/pytest -q
  • .venv/bin/flake8 sphinx/util/typing.py sphinx/util/inspect.py tests/test_util_typing.py tests/test_util_inspect.py tests/test_ext_autodoc_configs.py

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.

The follow-up looks great—thanks for restoring the expectations and normalizing the runtime differences in restify() and stringify_signature(). The optional-annotation helpers and broadened assertions cover the cross-version behaviour I was worried about, and the tests all line up again. Approved.

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.

2 participants