Add option for gslides markdown parser to raise on unsupported elements#54
Add option for gslides markdown parser to raise on unsupported elements#54ZmeiGorynych wants to merge 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
… egor/dev-1067-add-option-for-gslides-markdown-parser-to-raise-on # Conflicts: # gslides_api/element/table.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_markdown_features.py`:
- Around line 278-282: In test_exception_is_exported remove the stray orphaned
comment line ("# Some malformed markdown might legitimately fail") inside the
test function; edit the test_exception_is_exported method so it only contains
the docstring, the import aliasing of UnsupportedMarkdownError and the assertion
(i.e. reference the test_exception_is_exported function and
UnsupportedMarkdownError to locate and delete that leftover comment).
🧹 Nitpick comments (3)
gslides_api/agnostic/markdown_parser.py (2)
14-17: Consider removing redundantpassstatement.The
passstatement is unnecessary when a class body already contains a docstring.♻️ Suggested simplification
class UnsupportedMarkdownError(ValueError): """Raised when markdown contains elements that cannot be converted to the target format.""" - - pass
142-149: Consider centralizing error message construction.The error message pattern is repeated in three locations (lines 143-146, 269-272, 344-347). This could be simplified by adding a factory method to
UnsupportedMarkdownError, which would also address the Ruff TRY003 hint about long messages outside exception classes.♻️ Suggested refactor
Update the exception class:
class UnsupportedMarkdownError(ValueError): """Raised when markdown contains elements that cannot be converted to the target format.""" `@classmethod` def for_element(cls, element_type: str, node_name: str) -> "UnsupportedMarkdownError": return cls( f"Unsupported {element_type} element: {node_name}. " f"Use strict=False to skip unsupported elements." )Then replace the inline raises:
- raise UnsupportedMarkdownError( - f"Unsupported block element: {type(node).__name__}. " - f"Use strict=False to skip unsupported elements." - ) + raise UnsupportedMarkdownError.for_element("block", type(node).__name__)gslides_api/element/table.py (1)
152-215: Exposestricton higher-level table update helpers.Now that strict is plumbed into the cell write path, consider threading a
strictparameter throughcontent_update_requests(...)andcreate_element_from_markdown_requests(...)so table creation from markdown can opt into lenient behavior too.
| def test_exception_is_exported(self): | ||
| """Test that UnsupportedMarkdownError can be imported from the package.""" | ||
| from gslides_api import UnsupportedMarkdownError as ImportedError | ||
| assert ImportedError is UnsupportedMarkdownError | ||
| # Some malformed markdown might legitimately fail |
There was a problem hiding this comment.
Remove orphaned comment.
Line 282 contains an orphaned comment that appears to be a leftover from a previous test case.
🧹 Suggested fix
def test_exception_is_exported(self):
"""Test that UnsupportedMarkdownError can be imported from the package."""
from gslides_api import UnsupportedMarkdownError as ImportedError
assert ImportedError is UnsupportedMarkdownError
- # Some malformed markdown might legitimately fail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_exception_is_exported(self): | |
| """Test that UnsupportedMarkdownError can be imported from the package.""" | |
| from gslides_api import UnsupportedMarkdownError as ImportedError | |
| assert ImportedError is UnsupportedMarkdownError | |
| # Some malformed markdown might legitimately fail | |
| def test_exception_is_exported(self): | |
| """Test that UnsupportedMarkdownError can be imported from the package.""" | |
| from gslides_api import UnsupportedMarkdownError as ImportedError | |
| assert ImportedError is UnsupportedMarkdownError |
🤖 Prompt for AI Agents
In `@tests/test_markdown_features.py` around lines 278 - 282, In
test_exception_is_exported remove the stray orphaned comment line ("# Some
malformed markdown might legitimately fail") inside the test function; edit the
test_exception_is_exported method so it only contains the docstring, the import
aliasing of UnsupportedMarkdownError and the assertion (i.e. reference the
test_exception_is_exported function and UnsupportedMarkdownError to locate and
delete that leftover comment).
… egor/dev-1067-add-option-for-gslides-markdown-parser-to-raise-on
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.