Skip to content

✨ sphinx-needs: choose / when / otherwise directives, one of several branches by variant data - #2020

Open
chrisjsewell wants to merge 27 commits into
masterfrom
claude/elegant-darwin-2lj6bb
Open

chrisjsewell wants to merge 27 commits into
masterfrom
claude/elegant-darwin-2lj6bb

Conversation

@chrisjsewell

@chrisjsewell chrisjsewell commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

New choose, when and otherwise directives select one of several branches of content by variant data.
A choose holds when and otherwise directives (its branches) and comments, written in its own body:
it runs its when tests in order and the first true one is included;
otherwise is the optional default and comes last; when no test holds and there is no otherwise, nothing is rendered.
The other branches are never parsed, so their needs are never created, exactly as for a false if.
The names are those of choose / when / otherwise in XSLT, JSTL and MSBuild, which run their tests the same way.

.. choose::

   .. when:: var.arch == "arm"

      ARM content.

   .. when:: var.arch == "x86"

      x86 content.

   .. otherwise::

      Content for every other architecture.

MyST: ::::{choose} / :::{when} <condition> / :::{otherwise} (the outer fence longer than the inner ones), or backtick fences.

Closes #2011.

Why a container rather than elif / else siblings

For authors, a choose reads the way a choice is thought about: every alternative for one piece of content stands in one block,
in order, with the fallback visibly last. What is inside the block is the whole story.
A paragraph or a comment between two alternatives, or an include boundary, cannot detach a branch or turn a fallback into an
orphan, and a branch written inside a wrapper directive that passes its content through is refused with a warning, because every
when belongs to its choose and to nothing else. Mistakes are reported once, where they are made, and a mistake that makes a condition unevaluable never shows
the wrong variant: it skips the whole choice rather than falling through to the default, and a when that lost its condition is
reported rather than silently becoming the catch-all. A branch can hold anything a document can (sections, needs, includes, another choose), and the branches not taken
are never parsed, so no stray need or ID from another variant reaches the build. The same block builds the same way in
reStructuredText, in MyST and in ubCode (what still differs is registered in ubCode's divergence register), and in ubCode the editor fades the
branches that do not apply to the variant being built, so an author always sees which content is live. if is unchanged, so existing documents keep working as they are.

This is also why a container is sound where sibling elif / else are not. A docutils directive cannot see its source siblings,
so chained branches have to leave state behind for the next one, and every mechanism that needs (a marker node after every
branch including every plain if, a backward scan of the parent, a MyST fallback through the inliner's parent, a stripping
transform, a second strip for the need-node cache) exists only for that. A choose sees all its branches at once, decides once,
and returns only the winner, so none of it is needed. This supersedes the sibling-chain design of #1999; its applicable test
cases and its if.rst fix are salvaged here. ubCode implements the same contract (useblocks/ubcode#3783, companion issue
useblocks/ubcode#3763), so a document builds the same way in both tools.

Why choose / when / otherwise, and an explicit default

This PR first named the directives match / case, with a case without a condition as the default.
In Python and Rust a match takes a subject and every case is a pattern tested against it;
this construct has no subject and holds conditions, which is XSLT's, JSTL's and MSBuild's choose / when / otherwise,
so the names were changed (#2011, comment by @ubmarco), leaving match / case free for a subject-based form later.
The default is now a directive of its own: a when must have a condition,
so a condition forgotten on the last branch warns instead of silently making it the catch-all for every other variant.

How

  • when and otherwise do not parse their content: inside a choose body each returns a private placeholder
    (kind, condition, raw StringList, content offset, line, location, source file, and the node it is appended to);
    outside one it warns and is skipped. Both declare an optional argument, when so that a missing condition is reported by the
    choose with one warning (a required argument would make docutils refuse the directive with an error of its own),
    otherwise so that a condition on it can be refused.
  • choose parses its body into a detached node it never returns, accepts only placeholders and comments there,
    refuses a branch that is not a direct child of its body (one inside another directive, even a true if,
    a rst-class or a MyST {eval-rst} that hands its content's nodes through)
    and a branch supplied through an include (its source file differs from the choose's),
    checks the branches (every when has a condition, otherwise has none, is last and occurs at most once),
    and then evaluates the conditions in order;
    only the winner is parsed (nested_parse_with_titles), so headings in it are real sections.
    Later conditions are never evaluated.
  • Conditions go through evaluate_variant_condition, the evaluator moved out of IfDirective.run
    (if's messages and behaviour are byte-identical), so a condition means the same in if and when.
  • A depth counter in env.temp_data (raised only around the body parse, restored in a finally;
    the winner is parsed at 0) detects a when or otherwise outside a choose, including one loose in a branch's content.
  • Under MyST a directive's content_offset is relative to its own line, so the winner's offset is re-based there.
  • Content written directly in the body runs when the body is parsed; the needs it creates (the newest entries) are removed again.
  • A docutils message below the WARNING level (the INFO before the paragraph of a --- line, whatever the project's report level)
    is not taken for a reported error, so the content after it warns; a message docutils or MyST did report skips the choose
    without a second warning.
  • No transform, no add_node, no change to api/need.py, and if is untouched apart from the extraction.
  • Every warning the module reports carries an absolute source, as Sphinx's get_node_location does: docutils records an
    included file relative to the working directory whenever the two share their first two path components, which printed
    ../… locations and made the include rows of the suite depend on where the checkout lives (every Windows cell, and any
    checkout under /tmp).

Every mistake warns once under the new needs.choose type and skips the whole choose:
an argument on choose; content other than branches and comments (a line of punctuation such as --- included);
a branch inside another directive or supplied through an include; a comment that begins with when: or otherwise:
(a branch written with one colon, or without the space after ::, is a comment, and would hand the choice to the otherwise
or make the default vanish; the hint names the spelling of the syntax in use); no branch at all; a when without a condition;
an otherwise with one, a second one, or one that is not last;
unconfigured variant data (even for an otherwise-only choose); and a condition that cannot be evaluated,
so a mistake that makes a condition unevaluable, such as a misspelt key or a syntax error,
never renders a later branch or the default in its place.
A non-bool result warns and is used, as in if.
The documented limitations: a directive that produces no node (such as default-role, or a FALSE if, whose branches then
vanish) written directly in a choose is not detected, and neither is content a MyST substitution supplies; the non-need effects
of stray body content (a label, a needextend) stay. One inherited from if and documented on both pages: a sphinx-design tab-item directly inside the winner warns that its
parent should be a tab-set, because the winner is parsed into a detached container, exactly as a true if does.

Tests

tests/test_choose_directive.py (94 tests) and tests/doc_test/doc_choose_directive/:
the happy paths (first true wins with later invalid conditions never evaluated, otherwise, no otherwise,
needs and their line numbers, sections, nesting, an include inside a branch, a whole choose in an included file,
a choose in a need extracted by needextract);
a parametrised table of every mistake (one warning each, its text, [needs.choose] and its line,
in the included file for an include-supplied branch), including the check order
(no variant data and a misplaced otherwise give the one structural warning; two branch faults report the first in document order);
the rollback keeping the needs written before and after the choose and in an earlier document;
the depth counter restored when a body raises;
the MyST twin (colon and backtick fences, % and +++, nesting, includes, {eval-rst}, needextract, the MyST warnings);
the one-colon rows (when and otherwise, capitalised, with a space before the colon, without the space after ::, the MyST
% form, and the control comment that merely starts with the word);
the warning rows built from the source directory, where docutils records an included file relative to the working directory,
with a row for each exit that can locate a warning in an included file;
a check that no placeholder reaches any pickled doctree or the need-node cache;
the tab-item warning pinned inside a winning when and inside a true if;
and one expression table run through both .. if:: and .. when:: asserting identical verdicts and warnings.
Every row of #1999's warning table that has a counterpart in a container design is in that table.
tests/test_if_directive.py is unchanged and green; the two files also pass on the sphinx-7 cell (Sphinx 7.4, myst-parser 4.0.1),
and the full suite is green (1959 passed).

Docs

New docs/directives/choose.rst (after if in the directives index); if.rst gains a pointer to choose,
a label on its expression context, the non-bool warning it always emitted but never listed (salvaged from #1999),
and the tab-item note.

Changelog

docs/changelog.rst gains an Unreleased section with the entry (the :pr: reference is this PR's number).

Review

Two independent review passes on the match / case draft (one refuting fourteen named claims with its own probes and
mutations, one re-running every gate and mutation proof by hand) found no code defect; their findings (a silent skip on a
below-report-level docutils message, cases adopted through a transparent wrapper, four test gaps, three prose overclaims) were
fixed and validated. The rename and the explicit otherwise were then validated by two further review passes of the same shape on the renamed tree,
whose findings (the check order between the branch faults pinned, the refused otherwise text named, two comments and docs nits) are fixed in the commits after the docs commit.
The one-colon rule from the review below was then added in both engines, each validated by one of those reviewers.

Related

  • The Codecov upload condition (pushes to master never uploaded, so every pull request compared against a stale base and the
    reports flag read as a drop) landed as 🧪 CI: upload coverage on pushes to master too #2023, split out at review's request; this branch carries a revert of its own copy, so
    the squash nets to nothing for the workflows. This PR's reports status refreshes once it rebases or merges.

Merging

When squashing, strip the Co-authored-by trailer GitHub proposes (the commits carry the sandbox's git identity).

Follow-ups (not in this PR)

  • A condition whose truth test raises (__bool__ raising) aborts the build in when exactly as in if (bool(result) is
    outside the evaluator's try); inherited, tiny.
  • A directive that produces no node written directly in a choose body (a FALSE if, default-role), or content a MyST
    substitution supplies, is undetectable from the doctree; documented, and registered as a divergence from ubCode, which
    diagnoses any non-branch child.
  • A subject-based match / case (patterns, Python semantics) may follow; the names are left free for it.
  • A branch written with no colon at all (.. when cond) is a comment the choose cannot tell from one; the one-colon rule
    covers the likely slip (.. when: cond, .. when::cond) and leaves that one documented.
  • if warnings located in an included file still carry docutils' relative spelling, so if and when can name the same file
    differently; the same absolute-source treatment belongs in needif.py, outside this PR's rule not to touch if.

claude added 9 commits October 1, 2026 06:54
The evaluation of an `if` condition moves out of `IfDirective.run`
into a module-level function, `evaluate_variant_condition`,
which takes the directive name and the warning subtype as parameters.
`if` calls it with `"if"` / `"if"`, so its three messages,
its warning subtype and its behaviour are unchanged:
unconfigured variant data and an expression that raises skip the body,
and a result that is not a bool is warned about and then used as its truth value.

This is preparation for the `case` directive of `match` (#2011),
whose conditions must mean exactly what the same text means in `if`.
One function for both makes that true by construction
rather than by keeping two copies in step.

The `if` warnings were compared byte for byte against the previous code
over nine expressions, with and without variant data configured: identical.
A `match` holds `case` directives and comments.
The first `case` whose condition holds is included,
and a `case` with no condition is the default, which must be the last.
Only the content of that case is parsed,
so the needs in every other case are never created, as for a false `if`.

Why a container rather than `elif` / `else` siblings:
a directive cannot see its source siblings,
so sibling branches have to leave state behind for the next one,
and every mechanism that needs (a marker node after every branch,
a backward scan of the parent, a stripping transform,
a second strip for the need-node cache) exists only for that.
A `match` sees all its cases at once, decides once, and returns only the winner,
so none of it is needed and `if` stays as it is.

How it works:

- `case` does not parse its content.
  Inside a `match` body it returns a private placeholder
  carrying its condition, its raw content and its location;
  outside one it warns and is skipped.
- `match` parses its body into a detached node it never returns,
  accepts only placeholders and comments there,
  checks that there is at most one default and that it is last,
  and evaluates the conditions in order with `evaluate_variant_condition`,
  the evaluator `if` uses, so a condition means the same in both.
  Later conditions are not evaluated once a case is taken.
- Whether a `case` is in a `match` body is a depth counter in `env.temp_data`,
  raised only around the body parse;
  the winner is parsed at depth 0, so a `case` loose in a case's content is caught too.
- Under MyST a directive's `content_offset` is relative to its own line,
  so the winner's offset is re-based onto the `match`'s line there.
- Content written directly in the body runs its directives when the body is parsed,
  so any need the body parse created is removed again:
  case content is deferred, so such a need can only come from that mistake.

Every mistake warns once under the new `needs.match` subtype and skips the whole `match`:
an argument on `match`, content other than cases and comments,
no case, two defaults or a default before another case,
variant data that is not configured (even for a default-only `match`),
and a condition that cannot be evaluated, which also keeps the default from being taken,
so a typo in the intended case never renders the fallback in its place.
A result that is not a bool is warned about and used, as in `if`.
A `system_message` in the body was already reported by docutils or MyST,
so the `match` is skipped without a second warning.

The placeholder and the body node are not registered with `add_node`,
there is no transform, and `api/need.py` is not touched.
A doc project, `doc_match_directive`, holds the happy paths in reStructuredText:
the first true case wins while a later condition that is not Python,
and one naming an unknown key, are never evaluated (the build has no warning at all);
the default is taken, and nothing at all when no case holds and there is none;
the needs of the untaken cases are absent from the needs view
and the taken one records its source line;
a heading in the taken case is a section nested where it stands;
a nested match; an include that supplies the cases and one inside the taken case;
and a match in a need's content that `needextract` renders on another page.
No placeholder or body node reaches any pickled doctree or the need-node cache.

One parametrised table holds every mistake of the contract (#2011),
each asserted to warn exactly once, with its text, `[needs.match]`,
and the `<srcdir>/index.rst:N:` line of the offending directive or child,
and to render nothing of the match; the cases of #1999's warning table
map onto it one to one. A docutils error in the body is reported once, by docutils.
The MyST twin covers colon and backtick fences, `%` comments and `+++`,
needs in the taken and untaken case (with the true line, which the
content-offset re-basing gives), nesting, a default with trailing spaces,
needextract, and the MyST warnings; lines are asserted for the backtick
spellings only, since MyST reports a directive nested in a colon fence one
line late.

One table of expressions runs through both `.. if::` and `.. case::`
and asserts the same verdict and the same warning text for each,
so the two directives cannot drift into two condition languages.
`test_if_directive.py` and its projects are unchanged.
`docs/directives/match.rst` documents the directive pair after `if` in the
directives index: the RST and MyST spellings (colon fences first, with the rule
that an outer fence must be longer than the fences inside it, and `%` as the MyST
comment), the rules (the first true case wins and later conditions are not
evaluated, the default comes last, only cases and comments, the included case is
ordinary content), that a condition is exactly an `if` condition, the
one-line-condition pitfall, every warning under `needs.match`, and the one
limitation: a directive that produces no node, written directly in a `match`,
is not detected and still runs.

`if.rst` gains a one-line pointer to `match`, a label on its expression
context for that page to link to, and the warning `if` emits for a condition
whose result is not a bool, which it has always emitted and never listed.

The changelog gains an `Unreleased` section with the entry; its pull request
number is a placeholder until the pull request exists.
Every `case` of a `match` must now be written in the body of that `match`.
A `case` that an `.. include::` (or a MyST `{include}`) supplies
warns once under `needs.match`, at the `case` in the included file,
and the whole `match` is skipped, before any condition is evaluated.
An include inside the content of a case stays fine,
and so does a whole `match` written in an included file.

Why: one choice should be one directive in one place.
Splitting a choice across files is exactly what the sibling `elif` / `else`
design allowed and this one was chosen to avoid,
and ubCode, which implements the same contract, sees an include in a `match` body
as an invalid child (it splices includes as a separate tree, so it cannot see
the cases one supplies). Refusing it here keeps both engines on one rule.

How: a `case` records the source docutils or MyST attributes its line to
(`get_source_info()`), and the `match` compares it with its own.
Measured in both parsers, on Sphinx 9.1 and 7.4:
docutils reports the included file and its exact line;
MyST reports the included file too, with the line one late, as it does for
every line of an included file.

The doc project's include-supplied cases become two negative cases in the
warnings table, with a MyST twin, and a whole `match` in an included file is
added to the RST and MyST happy paths. `match.rst` states the rule and lists
the warning.
…quiet ones

Two ways to get a `match` wrong were not reported, which broke its promise
that every mistake warns once.

A line of one to three punctuation characters in the body, such as `---`
between two cases, makes docutils append an INFO message, which is below the
report level and never shown, BEFORE the paragraph it then makes of the line.
The match took every `system_message` child for one docutils had already
reported, and skipped itself in silence: under `-W` the build was green with
the content of the match gone. A message below the reporter's report level is
now passed over, so the paragraph after it is judged as any other content and
warns once, at its own line. A message at or above that level keeps the
silent skip, since docutils or MyST did show it.

A `case` inside a directive that returns the nodes of its own content
(a true `if`, a `rst-class` with content, a MyST `{eval-rst}` block) was
adopted as a case of the match, though it is not written in the match at all;
under `{eval-rst}` its RST content was then parsed as Markdown. A `case` now
records the node it is appended to (`state_machine.node`, which is docutils'
`RSTState.parent` and MyST's current node alike: measured identical in both
parsers on Sphinx 9.1 and 7.4), and the match refuses, with one warning at the
case, any case whose owner is not its own body. This is also the rule ubCode
applies to the same contract: anything but a case or a comment as a child of
a `match` is an invalid child. The depth counter still decides whether a
`case` is outside every `match`, so a case inside a `note` in a match still
gives exactly one warning, the match's `got <note>`.

Tests: a `---` and a `...` line, cases in a true `if` and in `rst-class`, and
a case in `{eval-rst}` in both MyST spellings each warn once and render nothing.
`match.rst` says that a case belongs directly in its match, lists both
mistakes, and states plainly that a FALSE `if` around cases returns nothing,
so those cases vanish without a warning unless the match is left with none.
The changelog entry's list of mistakes gains the case inside another directive
and the include refusal.
…er of `match`

Three behaviours of `match` were correct but unguarded: a reviewer's
mutations of each passed every test, and each mutant crashes or misleads a
real build.

- The rollback removes the NEWEST needs, the ones the body parse added.
  A new test writes a need before the stray-need `match` and one after it
  on the same page, and one in a document read earlier, and asserts all
  three survive while the stray one goes. Removing the oldest needs instead
  deletes a legitimate need and crashes `analyse_need_locations`.
- The depth counter is restored in a `finally`. A new test has a project
  directive catch an exception raised from inside a `match` body, and asserts
  that the loose `case` after it is still reported; without the `finally` it
  becomes a placeholder that reaches the writer.
- The structure is checked before the configuration. A new row has no variant
  data and a misplaced default, and expects the one structural warning at the
  case; checking the configuration first would report the other warning at the
  match and never parse the body.

Also a row for an argument-less `case` outside every match (the counterpart
of #1999's orphan `else`), in RST and MyST.

The warning about content in the body now names the first node the author
wrote: a need directive emits a target without a line before the need, so a
stray need was reported as `got <target>`; it now says `got <Need>`, at the
same line. A label written in the body is still reported as a target.

Prose: the one-line-condition sentence said a wrapped condition is always a
syntax error, but a break inside brackets evaluates; and "a typo in a
condition never renders the default" was not true for a typo that leaves a
valid, false condition. Both now say what holds: an unevaluable condition,
such as a misspelt key or a syntax error, never renders a later case or the
default in its place. Two test comments no longer claim to guard the
whitespace strips the parsers already perform.
…h` mistake

The match passed over a `system_message` in its body only when it was below
the reporter's report level, taking any other for an error docutils had shown.
A project whose `docutils.conf` sets `report_level: 1` shows INFO messages,
but as information rather than warnings: the INFO docutils puts before the
paragraph of a `---` line then made the match skip itself in silence again,
and `-W` stayed green with its content gone. A message is now taken as
reported only at WARNING or above (and at or above the report level), so the
paragraph is reported whatever the report level is. A new test sets the level
through the project's `docutils.conf`, as `sphinx-build` reads it.

The include check now runs before the direct-child check, so a case that
arrives through an include standing inside another directive (such as
`.. include:: … :parser: …`) gets the include wording.

`match.rst`: a wrapped condition also evaluates when the break is escaped with
a backslash, and the limitation note names MyST substitutions too: a
`{{ sub }}` in a `match` whose definition holds cases is expanded in place,
and those cases are taken without a warning.
@github-actions github-actions Bot added the pkg: sphinx-needs The sphinx-needs distribution (packages/sphinx-needs): its code, tests and docs label Oct 1, 2026
The `match` / `case` entry under `Unreleased` was stamped with a placeholder
until the pull request existed; it is #2020.
@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.47059% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.93%. Comparing base (1bae81a) to head (cdc6b1b).

Files with missing lines Patch % Lines
...nx-needs/src/sphinx_needs/directives/needchoose.py 96.13% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2020      +/-   ##
==========================================
+ Coverage   91.87%   91.93%   +0.06%     
==========================================
  Files         129      130       +1     
  Lines       18084    18323     +239     
==========================================
+ Hits        16614    16845     +231     
- Misses       1470     1478       +8     
Flag Coverage Δ
codelinks 93.74% <ø> (ø)
mounts 94.26% <ø> (ø)
pytests 91.58% <96.47%> (+0.10%) ⬆️
reports 88.01% <ø> (ø)
ub-test-reports 90.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

claude added 7 commits October 1, 2026 15:27
`match` and `case` name a construct with a subject, whose cases are
patterns tested against it (Python, Rust); the container built here has no
subject and holds conditions, which is XSLT's, JSTL's and MSBuild's
`choose` / `when`. The rename leaves the names `match` and `case` free for
a subject-based form later (#2011).

This commit only renames; behaviour is unchanged, and the default is still
a `when` with no condition until the next commit gives it a directive of
its own:

- `needmatch.py` becomes `needchoose.py`, with `ChooseDirective`,
  `WhenDirective`, `_BranchPlaceholder` and `_ChooseBody`, registered as
  `choose` and `when` next to `if`; the `env.temp_data` key becomes
  `sphinx_needs_choose_depth`.
- The warning type `needs.match` becomes `needs.choose`, sorted into
  `WarningSubTypes`, and every message names `choose` and `when`.
- The test module, its doc project and the docs page are renamed with
  `git mv` (`test_choose_directive.py`, `doc_choose_directive/`,
  `docs/directives/choose.rst`, label `choose`), as are the toctree entry,
  the pointer in `if.rst` and the changelog entry.

`if` is untouched: only the evaluator's docstring names `when` now.
…dition

A `when` with no condition was the default of its `choose`, so a `when`
whose condition had been forgotten silently became the catch-all for every
other variant. The default now has a directive of its own, `otherwise`,
and every `when` must have a condition. These are the only behaviour
changes; every other rule of `choose` is as before.

- `when` declares its argument optional, not required, so that the
  `choose` refuses a missing or blank condition itself, once, at the
  `when`: "'when' directive has no condition (use 'otherwise' for the
  default); the whole choose is skipped". A required argument would have
  made docutils reject the directive with an error of its own.
- `otherwise` declares an argument only to refuse one: "'otherwise'
  directive takes no condition; the whole choose is skipped". It must be
  the last branch, at most one per `choose` (the former default-position
  warnings, reworded for `otherwise`).
- A branch outside a `choose`, a branch inside another directive and a
  branch supplied through an include are warned about under the name of
  the directive it is written with; a `choose` with no branch says it has
  no 'when' or 'otherwise'.
- The `choose` argument is refused as before, but no longer described as
  reserved: `match` and `case` stay free for a form with a subject.

Both new refusals are structural, so they are checked after the body's
contents and before the configuration. `when` and `otherwise` share one
private base directive, and the placeholder carries the kind of its
branch, so that the `choose` can tell a `when` without a condition from
an `otherwise`.

Tests: every default in the module and its doc project is an `otherwise`
now; new rows for a `when` without a condition (RST, MyST backticks, and
a blank condition in MyST colons), an `otherwise` with a condition (RST
and MyST), an `otherwise` outside a `choose` (full text), inside a true
`if` and supplied through an include, and the check order of a `when`
without a condition when variant data is not configured.
`choose.rst` now describes the directives as built: `choose` runs its
`when` tests in order and the first true one is included; `otherwise` is
the optional default and comes last; when no test holds, nothing is
rendered. It names the precedent (XSLT, JSTL, MSBuild), says that a
`choose` has no subject, and lists the two new mistakes: a `when` without
a condition, and an `otherwise` with one. The MyST examples use
`{otherwise}`.

The content of the taken branch is parsed into a detached container, as
the body of a true `if` is, so a sphinx-design `tab-item` in it warns that
its parent should be a `tab-set`, even when the directive stands in one.
`choose.rst` says so in a note, with the remedy (put the `choose` inside
the `tab-item`), and `if.rst` gains a one-line bullet. A new test pins
the warning for both directives, at each `tab-item`, with the same text,
and that a `choose` inside a `tab-item` does not warn.

The changelog entry names the three directives and lists the two new
mistakes.
…docs nits

The order between the condition faults and the `otherwise` count and
position was what the code did but nothing pinned: moving the count and
position checks before the condition faults passed every test. New rows,
in reStructuredText and as MyST twins for the first two:

- two `otherwise` and then a `when` without a condition warn about the
  `when` (the condition faults, in document order, come first);
- an `otherwise` with a condition, a `when` and a bare `otherwise` warn
  about the condition, at the first `otherwise`;
- two bare `otherwise` and nothing else warn "more than one 'otherwise'"
  at the second.

An `otherwise` given a condition now names it ("'otherwise' directive
takes no condition, got '…'"), since the text may be content docutils
took for the argument because no blank line followed the directive;
`choose.rst` says so in its Warnings list.

The `tab-item` test no longer skips when sphinx-design is missing: it
imports it, so that the test fails if sphinx-design ever leaves the shared
`test` group, rather than silently no longer pinning the documented
limitation.

The two whitespace-only-argument guards are kept as the contract's, with
a comment saying that docutils and MyST already drop such an argument.
The `ChooseDirective` docstring and the changelog entry list "no branch at
all" among the mistakes, and `choose.rst` no longer claims that the names
mean exactly what they mean in XSLT, JSTL and MSBuild, which require a
`when`: it says that those run their tests the same way.
…ation

With no argument declared, docutils does not reject the directive: like
MyST, it moves the text into the content, which the `choose` would then
report only as a stray paragraph. The declaration stays, since it gives
the clearer message; only the comment is corrected.
The `_BranchDirective` docstring claimed docutils would reject an `otherwise`
with an error of its own if no argument were declared; measured, docutils and
MyST both fold the text into the content instead, where it would be taken
silently. The docstring now states the measured reason for each directive.
Two rows (and a MyST twin) pin that a choose with two mistakes reports the
first in document order: an otherwise with a condition before a when without
one reports the otherwise, and a when without a condition before a stray
paragraph reports the paragraph (the children are checked before the
condition faults). Both orders are the contract ubCode implements too.
@chrisjsewell chrisjsewell changed the title ✨ sphinx-needs: match / case directives, one of several branches by variant data ✨ sphinx-needs: choose / when / otherwise directives, one of several branches by variant data Oct 1, 2026
docutils writes the path of an included file with "/" on every platform
(utils.relative_path), so on Windows a warning located in such a file
starts with C:/... while the source directory is spelt C:\...; the
srcdir rewrite to <srcdir>/ missed it and the three choose rows that
locate a warning in an included file failed every Windows cell of CI.
build_warnings now rewrites the POSIX spelling too (a no-op elsewhere),
with a unit test that gives it a Windows source directory by hand.
@github-actions github-actions Bot added the pkg: sphinx-needs-testkit The shared test layer (packages/sphinx-needs-testkit); never published label Oct 1, 2026
The Codecov upload steps required github.event.pull_request.head.repo
to equal the repository, a guard against fork pull requests that has no
value on a push event, so since #2009 every upload on master was skipped
(measured on the run for cc0ffe0: all five "upload to Codecov" steps
skipped). Codecov then compared every pull request against the newest
ancestor with a report, d68d10d, 35 commits back and from before that
PR split sphinx-test-reports, so the reports flag read -0.74% on pull
requests that never touched the package. The guard now applies to pull
request events only; a push to master uploads under its own secrets.
@github-actions github-actions Bot added the pkg: workspace The repository as a whole: workflows, CI, release, docker, tooling, the workspace root label Oct 1, 2026
@ubmarco

ubmarco commented Oct 1, 2026

Copy link
Copy Markdown
Member

Reviewed at dadd8c4. This implements the proposal from #2011 completely: a when without a condition, an otherwise with one, a misplaced or second otherwise, and an argument on choose each warn and skip the whole choose. match/case stay free for later. if behaves exactly as before, since the extracted evaluator keeps its messages byte for byte, and the change only adds directives. The only compatibility surface is three new global directive names.

I also built against the head. A choose inside hlist renders, and inside a tab-set only the documented tab-item warning remains. A branch supplied through .. include:: is refused.

What is left are a few places where a mistake still lets the otherwise render, plus test portability.

1. [Medium] A branch written with one colon silently hands the choice to otherwise (correctness)

elif isinstance(child, nodes.comment):
continue

.. when: var.arch == "arm" is an RST comment, and it swallows the indented branch under it. Comments are accepted, so for arm the choose renders the otherwise, with no warning, and -W passes. That is exactly what the explicit otherwise exists to prevent. Refusing a comment whose text starts with when: or otherwise: would close it. This changes the contract, so ubCode needs the same rule.

2. [Medium] With report_level raised, a reported mistake falls through to otherwise (correctness)

reported = max(
self.state.document.reporter.report_level, Reporter.WARNING_LEVEL
)
if child["level"] < reported:
# never shown as a problem (below the report level, or below WARNING
# however low that level is set): judge what follows it instead
# (docutils puts an INFO before the paragraph of a `---` line)
continue

A system message below max(report_level, WARNING) is passed over.

  • MyST, with report_level: 3 in docutils.conf: it still logs Unknown directive type: 'wehn' through Sphinx, but the message node has level 2, so the choose moves on and renders the otherwise.
  • RST, with report_level: 4: .. wehn:: raises an error nobody sees, and -W passes with the otherwise rendered.

choose.rst says the choose is skipped all the same. Passing over INFO and DEBUG only keeps the --- case working. A message that the report level hid could then get a needs.choose warning of its own.

3. [Medium] The include rows of the suite depend on where the checkout lives (tests)

root = str(Path(srcdir))
# the POSIX spelling matters on Windows only, where `root` carries backslashes:
# an included file's location is written with "/" (see the docstring)
posix_root = root.replace("\\", "/")
for prefix in dict.fromkeys((root + os.sep, root + "/", posix_root + "/")):
text = text.replace(prefix, "<srcdir>/")

docutils records an included file with utils.relative_path(None, …). When the cwd and the file share their first two path components, that is a cwd-relative ../… path rather than an absolute one. The branch warning uses that raw string as its location (needchoose.py:141), so build_warnings never rewrites it to <srcdir>/.

  • From a checkout under /tmp, three rows of test_choose_warnings fail: branches from an include, a branch from an include after a true branch and an otherwise from an include.
  • The same follows on Windows wherever the checkout and %TEMP% both sit under C:\Users, which is the usual layout.
  • CI passes only because its workspace and temp directory have different roots.
  • Users see the same cwd-relative spelling in that warning.

Locating the warning with an absolute path, as Sphinx does for node locations, fixes both the tests and the warning.

4. [Low] The body rollback is partial (durability)

self.state.nested_parse(self.content, self.content_offset, body)
finally:
temp_data[_DEPTH_KEY] = depth
if needs is not None and len(needs) > before:
# the newest entries are the ones the body added: O(new needs)
for need_id in list(islice(reversed(needs), len(needs) - before)):
data.remove_need(need_id)

  • A needextend in the body registers itself while the body is parsed, and that registration survives. The choose warns that it is skipped, yet needs.json changes. The note in choose.rst admits this. Snapshotting the extends and needumls next to the needs would undo it cheaply.
  • The rollback runs after the try, not in its finally. A wrapper that catches an exception from the body therefore keeps the needs created before it, and those needs have no node in any doctree.

5. [Low] Invalid escape sequences in two new docstrings (correctness)

without knowing which temporary directory the fixture chose -- in its POSIX spelling
too, because docutils writes the path of an ``.. include::``\ d file with ``/`` on
every platform (``utils.relative_path``), so on Windows a warning located in such a
file starts with ``C:/…`` where the source directory is ``C:\…``;

\ d and C:\… in a non-raw docstring, here and in test_testkit_warnings.py:183, give a SyntaxWarning on every fresh compile under Python ≥ 3.12. Under -W error they give a SyntaxError. Making the docstrings raw fixes it. ruff does not select W605, which is why lint is green.

6. [Low] Land the Codecov change on its own, and fix three details (CI)

if: inputs.upload-coverage && github.repository == 'useblocks/sphinx-needs' && github.actor != 'dependabot[bot]' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository)
uses: codecov/codecov-action@v7
with:
token: ${{ secrets.CODECOV_TOKEN }}
name: ${{ inputs.codecov-name }}
flags: ${{ inputs.codecov-flag }}
files: ./coverage.xml # `file` was renamed to `files` in codecov-action v5
fail_ci_if_error: true

It is unrelated to choose, and a squash merge would bury it in a ✨ sphinx-needs commit, so I'd take up your offer to split it out. When it lands:

Reverts dadd8c4 on this branch: the change lands separately, with the
review corrections (fail_ci_if_error limited to pull requests, the token
descriptions, the history of the guard), so that a squash merge of this
branch does not bury a workspace-level CI change in a feature commit.
@read-the-docs-community

Copy link
Copy Markdown

Documentation build overview

📚 sphinx-codelinks | 🛠️ Build #34913981 | 📁 Comparing f685d61 against latest (16f247c)

  🔍 Preview build  

1 file changed
± changelog.html

chrisjsewell added a commit that referenced this pull request Oct 3, 2026
Every Codecov upload step is guarded by
`github.event.pull_request.head.repo.full_name == github.repository`, a
guard against fork pull requests (#1229, August 2024) that has no value
on a push event. So no push to master has uploaded a report since then;
measured on the CI run for `cc0ffe0`, all five "upload to Codecov" steps
are skipped.

Codecov compares a pull request against the newest ancestor that has a
report. That ancestor is now `d68d10d`, 35 commits back and from before
the sphinx-test-reports split (#2009), which moved statements out of the
`reports` flag, so `codecov/project/reports` reads as a coverage drop
(−0.74%) on every pull request, including ones that never touch the
package (#2020 is the current example). A looser threshold in
`codecov.yml` would hide this one delta while the base kept drifting;
the cause is the missing upload.

### What changes

- The fork guard applies to pull-request events only:
`(github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.full_name == github.repository)`. A
push to master uploads under the repository's own secrets; the
dependabot and repository checks are unchanged.
- `fail_ci_if_error` is limited to pull requests, so a Codecov outage
cannot turn master red.
- The two `CODECOV_TOKEN` secret descriptions say when the steps read
it.
- The reason is recorded in the step comment, with the history (#1229
wrote the guard, #2009 copied it onto the ub-test-reports step).

### What to expect

The first push to master after this merges uploads a fresh report; pull
requests opened or rebased after that compare against it. Pull requests
whose base is older, #2020 included, keep their current `reports` status
until they rebase or merge; it is not a finding about them.

Split out of #2020 at review's request; that branch reverts its copy of
the change.
claude added 7 commits October 3, 2026 09:36
`.. when: var.arch == "arm"` (one colon) is a comment in reStructuredText,
and so is `.. when::var.arch == "arm"` (no space after `::`): the comment
swallows the indented branch under it, comments are accepted in a
`choose`, and so for `arm` the `choose` rendered its `otherwise` with no
warning, which is exactly the mistake the explicit `otherwise` exists to
catch (review of #2020, point 1).

A comment in the body whose text, after leading whitespace, begins with
`when` or `otherwise` and a colon (case-insensitive) is now refused with
one warning at the comment: "'choose' directive has a comment that begins
with 'when:' (a branch written with one colon? write '.. when::
<condition>'); the whole choose is skipped" (for `otherwise`, "write
'.. otherwise::'"). It is a fault of a child, found in document order with
the others, before the no-branch check and the condition faults. A comment
that merely begins with the word ("when we migrate, drop this") is still
accepted. A MyST `%` comment follows the same rule.

A comment carries no line of its own (docutils gives it the line after
it, MyST the `choose`'s), so the warning's line is found in the content of
the `choose`: the first body-level comment line the rule matches, skipping
the lines docutils reads as `when` / `otherwise` directives and, under
MyST, the lines inside the fences of the body's directives.

Tests: `[when with one colon]`, `[otherwise with one colon]`, `[when
without the space after ::]`, the control `[a comment that starts with the
word when]` (no warning, its branch taken), the order pair `[a when without
a condition, then a when with one colon]`, and MyST `[when with one colon,
% comment]` with its control (a row without a text is now a control in the
MyST table). `choose.rst` and the changelog entry list the mistake.
Two rows pin that a comment beginning with `When:` or `when :` is refused
like `when:`, since ubCode mirrors the rule; and the docs bullet says what
a one-colon `otherwise` does (the default vanishes) rather than claiming
it hands the choice to the `otherwise`.
The warning about a comment that begins with `when:` or `otherwise:` told
a MyST author to write `.. when:: <condition>`, which is reStructuredText.
Under MyST it now says "write a '{when} <condition>' fence" (or "an
'{otherwise}' fence"); the reStructuredText spelling is unchanged. ubCode
gives the same hint by syntax.

The MyST row for `% when:` expects the fence hint, and a new MyST row pins
the `otherwise` form.
docutils records an included file with `utils.relative_path(None, path)`,
which is relative to the working directory whenever the two share their
first two path components: a checkout under /tmp building under /tmp, or,
on Windows, a checkout and %TEMP% both under C:\Users. A warning about a
branch in an included file (or about anything in a `choose` that stands in
one) then read `../…/branches.txt:1`, and the suite's rows that locate
such a warning failed wherever the checkout lived (review of #2020,
point 3).

Every location the `choose` directives report now goes through one helper
that makes the source absolute, as Sphinx does for a node's location: the
`choose`'s own warnings, the stray-branch warning, the condition warnings
the shared evaluator gives for a `when`, and the line the one-colon rule
finds. A node location is left to Sphinx, which makes it absolute itself.
The include check compares the two sources absolute as well.

Measured from a worktree under /tmp, with this code: the three include
rows failed before the change, located at
`../../../../../../../../../sn_test_build_data/…/branches.txt:1`, and pass
after it. A unit test pins the helper with a relative source.
docutils records an included file relative to the working directory only
when the two share their first two path components, so from a CI runner
the absolute-location fix was never exercised. The warning rows now build
from the source directory, where every include is recorded relative, and
two rows cover the other exits that can report a location in an included
file: an unevaluable condition inside an included choose, and a stray
branch. Each exit now has a row that fails when it loses the absolute
source.

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

pkg: sphinx-needs The sphinx-needs distribution (packages/sphinx-needs): its code, tests and docs pkg: sphinx-needs-testkit The shared test layer (packages/sphinx-needs-testkit); never published pkg: workspace The repository as a whole: workflows, CI, release, docker, tooling, the workspace root

Projects

None yet

Development

Successfully merging this pull request may close these issues.

✨ choose / when / otherwise directives: one of several content branches by variant data

3 participants