Most malformed input to needpie and needbar does not produce a Sphinx warning.
It raises out of the doctree-resolved handler, so sphinx-build exits 2 and no
output is produced for the whole project — not just for the offending chart.
That is out of step with the rest of the extension: the filter engine already warns and
degrades. A bad :filter-func: reference, for instance, is handled properly at
sphinx_needs/directives/needpie.py:177-185 — log_warning(..., "filter_func", location=node) and the node is dropped, build continues.
Measured
Everything below was measured at 3fc55cd with sphinx-build -b html -E, one small
project per case, matplotlib 3.11.1, Sphinx 9.0.4. rc is the exit code.
| input |
rc |
what the user sees |
needpie :labels: count ≠ content lines (either direction) |
2 |
ValueError: 'labels' must be of length 'x', not 2 |
needpie :explode: count ≠ content lines |
2 |
ValueError: 'explode' must be of length 'x', not 1 |
needpie :explode: abc |
2 |
ValueError: could not convert string to float: 'abc' (during reading) |
needpie :colors: notacolor |
2 |
ValueError: Invalid RGBA argument: 'notacolor' |
needpie :text_color: nope |
2 |
matplotlib color error |
needpie :style: no_such_style |
2 |
OSError: ... not a valid package style |
needpie :filter-func: returning non-numbers |
2 |
an ERROR log line, then could not convert string to float |
needpie content line that is an RST comment (.. x) |
2 |
SyntaxError: invalid syntax |
needbar rows of differing length (an empty line counts as a row) |
2 |
Exception: Needbar - file 'chart' - line '4': each content line must have the same length |
needbar :xlabels: count ≠ columns |
2 |
Exception: ... length of xlabels: 1 is not equal with sum of columns: 2 |
needbar :ylabels: count ≠ rows |
2 |
Exception: ... length of ylabels: 1 ... sum of rows: 2 |
needbar :colors: notacolor |
2 |
ValueError: 'facecolor' or 'color' argument must be a valid color |
needbar :style: no_such_style |
2 |
OSError |
needbar a filter containing a comma, with the default separator |
2 |
SyntaxError: '[' was never closed |
| invalid filter syntax in any cell or content line |
2 |
SyntaxError: invalid syntax |
Nine of the fifteen surface as a raw matplotlib message with no file, no line and no
mention of Sphinx-Needs. The reader has to guess which of their charts it was.
Where it happens
Raised by matplotlib, uncaught:
sphinx_needs/directives/needpie.py:153 (matplotlib.style.use)
sphinx_needs/directives/needpie.py:254 (axes.pie, i.e. labels / explode / colors / text color)
sphinx_needs/directives/needbar.py:328 (matplotlib.style.use)
sphinx_needs/directives/needbar.py:375 (axes.bar)
Raised deliberately, as a bare Exception:
sphinx_needs/directives/needbar.py:234 (ragged grid)
sphinx_needs/directives/needbar.py:272 (xlabels arity)
sphinx_needs/directives/needbar.py:278 (ylabels arity)
These three at least name the file and line in their own message, but they still stop
the build.
Three smaller defects in the same area
I would fold these into the same change rather than file them separately:
- Rotation options silently drop anything that is not
str.isdigit()
(needbar.py:428, :449, :458). :xlabels_rotation: -45 and
:sum_rotation: 45.5 are ignored, with exit code 0 and no warning at all.
needbar.py:362 uses print() where a warning is meant — it never reaches the
Sphinx logger, cannot be suppressed and carries no location. Its condition is also
effectively unreachable, because the color list is extended with ten default colors
three lines earlier (:359), so multi > 1 needs more than ten more rows than
colors. The documented "you will get a warning" therefore never happens.
logger.error(...) without a location or a subtype
(needpie.py:199, :205, :211, :215). These four messages have no file:line
and no [needs.*] suffix, so they cannot be traced or suppressed like every other
message the extension emits.
Proposal
Validate up front, then warn and render nothing, exactly as the :filter-func: path
already does:
- check
:labels: / :explode: / :xlabels: / :ylabels: arity, the grid shape, and
the :colors: / :text_color: / :style: values before handing anything to
matplotlib;
- on a problem,
log_warning(logger, <message naming the option and the two counts>, "chart", location=node) and replace the node with nothing (or with the
no_needs_found_paragraph that needpie already uses for an empty chart);
- give the rotation options the same treatment, accepting any number and warning on
anything else;
- give the four
logger.error calls a subtype and location=node, or turn them into
log_warning.
A new subtype name would be needed — needs.chart seems natural next to the existing
needs.filter, needs.filter_func and needs.mpl.
The compatibility question
This is a behaviour change and I do not think it can be made without one: a project
that fails today would start succeeding with warnings. Two things soften it:
- for anyone building with
-W, nothing changes in outcome — the build still fails,
with a better message and a location;
- for everyone else, the alternative today is a project that produces no output at all
because of one mistyped color.
Would you want it unconditional, or behind a configuration value
(needs_chart_errors = "warn" | "raise")? I am happy to write it either way, but I did
not want to guess. The measurements above — and the small chart fixes in the companion
PR — came out of the same review.
Most malformed input to
needpieandneedbardoes not produce a Sphinx warning.It raises out of the
doctree-resolvedhandler, sosphinx-buildexits 2 and nooutput is produced for the whole project — not just for the offending chart.
That is out of step with the rest of the extension: the filter engine already warns and
degrades. A bad
:filter-func:reference, for instance, is handled properly atsphinx_needs/directives/needpie.py:177-185—log_warning(..., "filter_func", location=node)and the node is dropped, build continues.Measured
Everything below was measured at
3fc55cdwithsphinx-build -b html -E, one smallproject per case, matplotlib 3.11.1, Sphinx 9.0.4.
rcis the exit code.needpie:labels:count ≠ content lines (either direction)ValueError: 'labels' must be of length 'x', not 2needpie:explode:count ≠ content linesValueError: 'explode' must be of length 'x', not 1needpie:explode: abcValueError: could not convert string to float: 'abc'(during reading)needpie:colors: notacolorValueError: Invalid RGBA argument: 'notacolor'needpie:text_color: nopeneedpie:style: no_such_styleOSError: ... not a valid package styleneedpie:filter-func:returning non-numbersERRORlog line, thencould not convert string to floatneedpiecontent line that is an RST comment (.. x)SyntaxError: invalid syntaxneedbarrows of differing length (an empty line counts as a row)Exception: Needbar - file 'chart' - line '4': each content line must have the same lengthneedbar:xlabels:count ≠ columnsException: ... length of xlabels: 1 is not equal with sum of columns: 2needbar:ylabels:count ≠ rowsException: ... length of ylabels: 1 ... sum of rows: 2needbar:colors: notacolorValueError: 'facecolor' or 'color' argument must be a valid colorneedbar:style: no_such_styleOSErrorneedbara filter containing a comma, with the default separatorSyntaxError: '[' was never closedSyntaxError: invalid syntaxNine of the fifteen surface as a raw matplotlib message with no file, no line and no
mention of Sphinx-Needs. The reader has to guess which of their charts it was.
Where it happens
Raised by matplotlib, uncaught:
sphinx_needs/directives/needpie.py:153(matplotlib.style.use)sphinx_needs/directives/needpie.py:254(axes.pie, i.e. labels / explode / colors / text color)sphinx_needs/directives/needbar.py:328(matplotlib.style.use)sphinx_needs/directives/needbar.py:375(axes.bar)Raised deliberately, as a bare
Exception:sphinx_needs/directives/needbar.py:234(ragged grid)sphinx_needs/directives/needbar.py:272(xlabels arity)sphinx_needs/directives/needbar.py:278(ylabels arity)These three at least name the file and line in their own message, but they still stop
the build.
Three smaller defects in the same area
I would fold these into the same change rather than file them separately:
str.isdigit()(
needbar.py:428,:449,:458).:xlabels_rotation: -45and:sum_rotation: 45.5are ignored, with exit code 0 and no warning at all.needbar.py:362usesprint()where a warning is meant — it never reaches theSphinx logger, cannot be suppressed and carries no location. Its condition is also
effectively unreachable, because the color list is extended with ten default colors
three lines earlier (
:359), somulti > 1needs more than ten more rows thancolors. The documented "you will get a warning" therefore never happens.
logger.error(...)without a location or a subtype(
needpie.py:199,:205,:211,:215). These four messages have no file:lineand no
[needs.*]suffix, so they cannot be traced or suppressed like every othermessage the extension emits.
Proposal
Validate up front, then warn and render nothing, exactly as the
:filter-func:pathalready does:
:labels:/:explode:/:xlabels:/:ylabels:arity, the grid shape, andthe
:colors:/:text_color:/:style:values before handing anything tomatplotlib;
log_warning(logger, <message naming the option and the two counts>, "chart", location=node)and replace the node with nothing (or with theno_needs_found_paragraphthatneedpiealready uses for an empty chart);anything else;
logger.errorcalls a subtype andlocation=node, or turn them intolog_warning.A new subtype name would be needed —
needs.chartseems natural next to the existingneeds.filter,needs.filter_funcandneeds.mpl.The compatibility question
This is a behaviour change and I do not think it can be made without one: a project
that fails today would start succeeding with warnings. Two things soften it:
-W, nothing changes in outcome — the build still fails,with a better message and a location;
because of one mistyped color.
Would you want it unconditional, or behind a configuration value
(
needs_chart_errors = "warn" | "raise")? I am happy to write it either way, but I didnot want to guess. The measurements above — and the small chart fixes in the companion
PR — came out of the same review.