Skip to content

fix(pptx): return a placeholder when a chart read fails instead of None - #2294

Open
Guillermo Dols (gdols) wants to merge 1 commit into
microsoft:mainfrom
gdols:fix/pptx-chart-none-return
Open

fix(pptx): return a placeholder when a chart read fails instead of None#2294
Guillermo Dols (gdols) wants to merge 1 commit into
microsoft:mainfrom
gdols:fix/pptx-chart-none-return

Conversation

@gdols

Copy link
Copy Markdown

PptxConverter._convert_chart_to_markdown can return None, and the caller concatenates the result straight onto md_content, so one unreadable chart fails the conversion of the entire presentation.

The path

Both exception handlers in that method produce the same [unsupported chart] placeholder, but the ValueError one is guarded by a message check:

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"   # <- only this message returns
except Exception:
    # Catch any other exceptions that might occur
    return "\n\n[unsupported chart]\n\n"

A ValueError carrying any other message falls out of the handler, the function ends, and None comes back. The call site does not check it:

if shape.has_chart:
    md_content += self._convert_chart_to_markdown(shape.chart)

MarkItDown._convert catches the resulting TypeError and, with no other converter accepting a .pptx, re-raises it as FileConversionException:

PptxConverter threw TypeError with message: can only concatenate str (not "NoneType") to str

So the failure mode is not a chart rendered badly — it is the whole deck lost, including every slide that converted fine.

The change

Since both handlers returned the same string, the ValueError branch was only ever narrowing which errors got handled. Collapsing them into a single except Exception keeps the output identical for the case that already worked and removes the path that returns None.

markitdown-ocr carries its own copy of this method with the same defect, so it gets the same fix.

Tests

packages/markitdown/tests/test_pptx_chart_errors.py:

  • the method returns a string for an arbitrary ValueError as well as for unsupported plot type (parametrized);
  • a real one-slide deck whose chart raises still converts, and carries the placeholder;
  • control: an ordinary chart still converts to a table.

Two of the four fail on main and all four pass with this change. The rest of the suite is unaffected and black reports no changes.

Note

This does not overlap #2264, which reworks the body of the try for scatter and bubble series; the handler tail this touches is untouched there.

_convert_chart_to_markdown returns "[unsupported chart]" from both of its
exception handlers, but the ValueError handler returned it only when the
message contained "unsupported plot type". Every other ValueError reached the
end of that handler and fell off the function, so the method returned None.

The caller concatenates the result onto md_content unguarded, so the None
raises TypeError: can only concatenate str (not "NoneType") to str. That is
caught by MarkItDown._convert and re-raised as FileConversionException, so a
single unreadable chart loses the whole presentation rather than one shape.

Both handlers produced the same string, so collapsing them into one
except Exception preserves the output and removes the None path. The same
defect is present in the copy carried by markitdown-ocr.
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