Skip to content

needpie / needbar: ordinary user errors end the whole build instead of warning #1771

Description

@chrisjsewell

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-185log_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:

  1. 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.
  2. 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.
  3. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions