Skip to content

fix(markers): correct Marker.evaluate :raises: from docs (and subclass KeyError for backcompat) - #1276

Merged
henryiii merged 1 commit into
pypa:mainfrom
r266-tech:docs-markers-evaluate-raises-keyerror
Jun 23, 2026
Merged

henryiii merged 1 commit into
pypa:mainfrom
r266-tech:docs-markers-evaluate-raises-keyerror

Conversation

@r266-tech

Copy link
Copy Markdown
Contributor

Resolves the Marker.evaluate item from your review checklist in #1239:

Marker.evaluate documents :raises UndefinedEnvironmentName: but the code raises KeyError (tests pin KeyError) … docs/markers.rst imports it pointlessly. Decide: either raise it or remove it from docs/__all__.

UndefinedEnvironmentName is exported in markers.py __all__ and defined as a class, but it is never raised anywhere in the source. evaluate() reads environment[environment_key] directly (markers.py:273/:278), so a missing environment key surfaces as a bare KeyError, and the tests pin exactly that (tests/test_markers.py:505/:508). The docstring has been wrong since the exception became dead.

This PR takes the low-risk, behavior-preserving fork of your decide:

  • correct the evaluate() docstring :raises: from UndefinedEnvironmentName to KeyError (matches the test-pinned behavior), and
  • drop the now-pointless UndefinedEnvironmentName import from the docs/markers.rst doctest (it is never referenced again, so the doctest still runs).

I deliberately kept the __all__ export and the class definition, and did not change evaluate() to actually raise UndefinedEnvironmentName — both of those would be behavior / public-API changes. If you'd rather go the other way (make evaluate() raise UndefinedEnvironmentName so the exported class becomes meaningful and the original docstring becomes true), I'm happy to send that version instead — just say the word.

The sibling :raises UndefinedComparison: is genuinely raised (markers.py:234) and is left untouched.

@henryiii

Copy link
Copy Markdown
Contributor

We could make UndefinedEnvironmentName a KeyError subclass instead of ValueError, and actually raise it.

…r missing env keys

Per @henryiii's review on pypa#1239: make UndefinedEnvironmentName a KeyError
subclass (was ValueError) and actually raise it from Marker.evaluate(), so the
exported exception becomes meaningful and the documented :raises: is true again.

Missing environment keys previously surfaced as a bare KeyError straight from
the dict lookup in _evaluate_markers; tests pinned that KeyError. Because
UndefinedEnvironmentName now subclasses KeyError, that contract is preserved
(existing 'with pytest.raises(KeyError)' assertions still pass and external
'except KeyError' keeps working) while callers can catch the specific type.

refs pypa#1239
@r266-tech
r266-tech force-pushed the docs-markers-evaluate-raises-keyerror branch from 0cfa0aa to 05b6ea5 Compare June 20, 2026 16:41
@r266-tech

Copy link
Copy Markdown
Contributor Author

Done — force-pushed the branch to exactly that. UndefinedEnvironmentName now subclasses KeyError (was ValueError) and Marker.evaluate() raises it from the environment lookup in _evaluate_markers (via a small _lookup_environment helper, raise UndefinedEnvironmentName(key) from None), so the exported exception is finally meaningful and the :raises UndefinedEnvironmentName: docstring is true again.

Since it's a KeyError subclass, the pinned pytest.raises(KeyError) cases (and any downstream except KeyError) keep working unchanged; I added a regression test asserting the specific type is raised, that args == ("extras",), and that it's still catchable as KeyError. The diff is now just the exception change — I dropped the earlier docstring/doctest-import commits since they're moot under this approach. UndefinedComparison is left untouched.

@henryiii henryiii changed the title docs(markers): correct Marker.evaluate :raises: to the test-pinned KeyError fix(markers): correct Marker.evaluate :raises: from docs (and subclass KeyError for backcompat) Jun 22, 2026
@henryiii
henryiii merged commit b19df94 into pypa:main Jun 23, 2026
69 checks passed
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