fix(pptx): prevent crash in PptxConverter when chart title lacks a text frame - #2194
Merged
Merged
Conversation
…xt frame Chart title text_frame can be None if the title is set directly or programmatically in python-pptx. Added a None check before attempting to read text. Signed-off-by: 辰言 <oncwnuIWp30GguOyJ615Fqj8H-yc@git.weixin.qq.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Only minor metadata and whitespace nits remain.
Pull request overview
Preserves PPTX chart conversion when a chart title lacks a text frame.
Changes:
- Guards title extraction in standard and OCR converters.
- Adds regression tests for both paths.
File summaries
| File | Review |
|---|---|
packages/markitdown/tests/test_module_misc.py |
Adds regression coverage; remove trailing whitespace. |
packages/markitdown/src/markitdown/converters/_pptx_converter.py |
Guards title extraction; PR metadata should clarify existing fallback behavior. |
packages/markitdown-ocr/tests/test_pptx_converter.py |
Adds OCR regression coverage. |
packages/markitdown-ocr/src/markitdown_ocr/_pptx_converter_with_ocr.py |
Guards OCR chart-title extraction. |
Review details
Suppressed comments (2)
packages/markitdown/src/markitdown/converters/_pptx_converter.py:313
- The surrounding
except Exceptionalready catches thisAttributeErrorand returns[unsupported chart], so the previous behavior loses chart content but does not let the converter crash as the PR title and description state. Please update the PR metadata to describe preserving chart conversion, or add an integration case demonstrating an exception path that actually escapes.
if chart.has_title and chart.chart_title.text_frame is not None:
packages/markitdown/tests/test_module_misc.py:1028
- These newly added blank lines contain trailing spaces; remove the whitespace so the test file remains clean and passes whitespace-sensitive linting.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 3, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR resolves a crash where
PptxConverter(andPptxConverterWithOCR) throws anAttributeErrorwhen converting presentations containing charts whose titles do not have atext_frame.According to the python-pptx documentation,
chart_title.text_frameisNoneif the title was set directly or programmatically without using the rich text frame API.Fix
text_frame is not Nonecheck before readingchart.chart_title.text_frame.textin both converters.PptxConverterandPptxConverterWithOCRto verify correct behavior.