Skip to content

test(alter): name the explicit-context parent something defined nowhere else - #1645

Draft
dimitri-yatsenko wants to merge 1 commit into
masterfrom
test/alter-explicit-context-name
Draft

dimitri-yatsenko wants to merge 1 commit into
masterfrom
test/alter-explicit-context-name

Conversation

@dimitri-yatsenko

Copy link
Copy Markdown
Member

Review follow-up to #1617, from @MilagrosMarin's note. test_alter_explicit_context said Parent resolves only in the explicit context, but the test module imports Parent, so it resolves in the caller's namespace too. The test still told the two apart, but its comment did not hold. The reference is now Upstream, which nothing else defines, so it can resolve only through the explicit context. The alter tests pass (8).

…re else

Review follow-up to #1617. test_alter_explicit_context said `Parent` resolves only in the explicit context, but the test module imports `Parent`, so it resolves in the caller's namespace too. The reference is now `Upstream`, which nothing else defines, so the comment holds.
@dimitri-yatsenko dimitri-yatsenko added this to the v2.4 milestone Oct 21, 2026
@dimitri-yatsenko dimitri-yatsenko added the documentation Issues related to documentation label Oct 21, 2026

@MilagrosMarin MilagrosMarin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Upstream is defined neither in test_alter.py nor in the generated pipeline module, so it can only resolve through the explicit context, and the comment now holds.

Two tiny, non-blocking notes:

  1. "Defined nowhere else" is slightly broad: other test modules (test_cascade_delete.py, test_dependencies.py, test_trace.py, …) define an Upstream. None of them is in scope here, so "defined in neither this module nor the pipeline module" would be exact.
  2. The docstring says the explicit context "takes precedence over the declaration context". With a name only the explicit context defines, the test shows that the context is used, not that it wins a conflict. Mapping a name the pipeline module also defines (e.g. Item) to a different table would test precedence itself, maybe as a follow-up; the previous version didn't test it either.

@dimitri-yatsenko
dimitri-yatsenko marked this pull request as draft October 21, 2026 16:46

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Issues related to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants