Skip to content

Conversation

@Dev-iL
Copy link
Collaborator

@Dev-iL Dev-iL commented Jul 15, 2025

This is a different fix for the same issue addressed in #53364, which was developed in parallel.

The main improvement are:

  • Eliminate # type: ignore[attr-defined]
  • isinstance branching happens at an earlier scope (so fewer times overall).

Also (according to copilot):

  1. Type Safety:
  • The suggested approach only processes nodes that are isinstance(node, nodes.Element), which is correct because only Element nodes have .attributes, .children, and .rawsource.
  • The existing approach uses node.get(...) and then checks isinstance(node, nodes.Element) later, which is less clear and could allow non-Element nodes to be processed incorrectly.
  1. Clarity and Correctness:
  • The suggested approach makes it explicit that only Element nodes with the substitution option are processed, matching the docutils node model.
  • The existing approach could process nodes that are not Element, leading to possible attribute errors or incorrect behavior.
  1. Efficiency:
  • The suggested approach avoids unnecessary checks and only runs the substitution logic where it is valid.

Copilot AI review requested due to automatic review settings July 15, 2025 10:33

This comment was marked as outdated.

@Dev-iL Dev-iL force-pushed the Dev-iL/2507/sphinx-types branch from 3d30202 to 8bb623b Compare July 15, 2025 10:46
@Dev-iL Dev-iL requested a review from Copilot July 15, 2025 10:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the substitution extension to perform isinstance checks at an earlier scope, reduces redundant branching, and removes a type ignore on the rawsource assignment.

  • Move isinstance check for nodes.Element before attribute lookup
  • Consolidate per-child isinstance(nodes.Text) logic
  • Remove # type: ignore[attr-defined] on node.rawsource
Comments suppressed due to low confidence (1)

devel-common/src/sphinx_exts/substitution_extensions.py:85

  • [nitpick] The removal of the type ignore on rawsource assignment changes behavior for some node types. Add tests to verify that rawsource is correctly set for all relevant nodes to prevent regressions in highlighting.
                node.rawsource = node.astext()

@Dev-iL Dev-iL force-pushed the Dev-iL/2507/sphinx-types branch from 8bb623b to 5672f32 Compare July 15, 2025 11:12
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

Nice, looks good! Let's wait for CI

@amoghrajesh amoghrajesh requested review from eladkal and potiuk July 15, 2025 12:00
@amoghrajesh amoghrajesh merged commit 12c317b into apache:main Jul 15, 2025
102 checks passed
@potiuk
Copy link
Member

potiuk commented Jul 15, 2025

Nice... I've been fighting with it in my Python 3.13 PR as well :D

gopidesupavan pushed a commit to gopidesupavan/airflow that referenced this pull request Jul 26, 2025
Trying to fix failing mypy devel-common
potiuk pushed a commit that referenced this pull request Jul 26, 2025
Trying to fix failing mypy devel-common

Co-authored-by: Dev-iL <Gid.CTO+github@gmail.com>
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