Skip to content

fix(pptx): report an unreadable chart instead of losing the presentation - #2510

Open
Lukas (L4XB) wants to merge 1 commit into
microsoft:mainfrom
L4XB:fix/pptx-chart-does-not-lose-the-deck
Open

Lukas (L4XB) wants to merge 1 commit into
microsoft:mainfrom
L4XB:fix/pptx-chart-does-not-lose-the-deck

Conversation

@L4XB

Copy link
Copy Markdown

Summary

PptxConverter._convert_chart_to_markdown has a branch that can return nothing,
and the caller concatenates the result straight into the document:

except ValueError as e:
    # Handle the specific error for unsupported chart types
    if "unsupported plot type" in str(e):
        return "\n\n[unsupported chart]\n\n"
except Exception:
    # Catch any other exceptions that might occur
    return "\n\n[unsupported chart]\n\n"

A ValueError whose message is anything else matches the first clause, fails
the if, and falls off the end of the function. except Exception is never
reached, because except ValueError already matched. So the function returns
None, and md_content += self._convert_chart_to_markdown(shape.chart) raises.

One chart the converter cannot read costs the entire presentation:

FileConversionException: File conversion failed after 1 attempts:
 - PptxConverter threw TypeError with message: can only concatenate str (not "NoneType") to str

The trigger this is reachable through

A non-numeric point in a chart's numeric cache. #N/A is what a linked
worksheet leaves behind, and python-pptx parses the cache eagerly, so the whole
series raises:

File ".../pptx/chart/series.py", line 83, in values
    return tuple(iter_values())
ValueError: could not convert string to float: '#N/A'

Measured on a two-slide deck, chart on slide 1 and plain text on slide 2, with
one <c:v>20.0</c:v> rewritten to <c:v>#N/A</c:v>:

beforeafter
FileConversionException:
PptxConverter threw TypeError
with message: can only
concatenate str (not
"NoneType") to str
<!-- Slide number: 1 -->
# Quarterly revenue

### Chart

| Category | Revenue |
|---|---|
| Q1 | None |
...
<!-- Slide number: 2 -->
# Outlook
Everything after the chart lives here.

The title, the second slide and the rest of the deck have nothing to do with
that chart, and were being thrown away with it.

Change

Drop the special-cased ValueError clause. Both branches returned the same
placeholder string, so it never did anything except open the hole. What remains
is the except Exception that was already there, and now there is no path out
of the function that returns nothing.

Read each series' values through a small helper. The categories and the
series names are still readable when the cache is not, so a chart with one
#N/A keeps its table instead of collapsing to [unsupported chart]. A series
whose values cannot be read renders the same empty cells a short series already
renders through sv[idx] if idx < len(sv) else None, so the row shape is
unchanged.

Not changed: how None renders in a cell. map(str, row) already prints it for
a short series and that is outside this fix.

Tests

packages/markitdown/tests/test_pptx_chart.py, 5 tests: a readable chart is
still a table; an #N/A point keeps the deck and the table's shape; a chart
that fails for another reason becomes [unsupported chart] and the deck
survives; an unsupported plot type still becomes [unsupported chart]; and the
converter never returns None, since the caller concatenates it.

$ pytest packages/markitdown/tests/test_pptx_chart.py -q
5 passed

# same tests, converter reverted to main
3 failed, 2 passed

Whole suite, unchanged either way apart from the new file:

$ pytest packages/markitdown/tests/ -q
897 passed, 14 skipped

black (the repo's pre-commit hook) clean on both files.

`_convert_chart_to_markdown` catches `ValueError` and returns the placeholder
only when the message contains "unsupported plot type". Any other `ValueError`
matches that clause, fails the `if`, and falls off the end of the function, so
it returns `None` -- and the caller does `md_content += ...`, which raises
`TypeError: can only concatenate str (not "NoneType") to str`. One chart the
converter cannot read therefore costs the whole deck.

The most ordinary trigger is a non-numeric point in a chart's numeric cache: an
`#N/A` left behind by the linked worksheet makes python-pptx raise
`ValueError: could not convert string to float: '#N/A'` from `series.values`,
which it parses eagerly for the whole series.

```
FileConversionException: File conversion failed after 1 attempts:
 - PptxConverter threw TypeError with message: can only concatenate str (not "NoneType") to str
```

Both branches returned the same placeholder, so drop the special-cased
`ValueError` clause; `except Exception` already says what was meant, and now
there is no path out of the function that returns nothing.

Read each series' values through a small helper as well. The categories and the
series names survive an unreadable cache, so a chart with one `#N/A` keeps its
table instead of collapsing to `[unsupported chart]`. A series whose values
cannot be read renders the same empty cells a short series already renders.
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.

1 participant