Add experimental Markdown importer - #160
Conversation
|
🔍 OpenCodeReview found 19 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| class ParserConfig(TypedDict, total=False): | ||
| """Options controlling which Markdown constructs are recognized.""" |
There was a problem hiding this comment.
The ParserConfig TypedDict documents feature toggles but doesn't specify what the default values are. Implementation uses True for most features, but this is only discoverable by reading the parse() method. Consider adding inline comments or a class-level docstring noting that boolean options default to True and list options default to [].
Suggestion:
| class ParserConfig(TypedDict, total=False): | |
| """Options controlling which Markdown constructs are recognized.""" | |
| class ParserConfig(TypedDict, total=False): | |
| """Options controlling which Markdown constructs are recognized. | |
| All boolean options default to ``True``; list options default to ``[]``. | |
| """ |
| for block in content_state.get("blocks", []): | ||
| kept = self._apply_block_rule(copy.deepcopy(block), block_rules) |
There was a problem hiding this comment.
Unnecessary deep copy when no block rules match. The copy.deepcopy(block) at line 113 is always called, but when no block rules match (line 188-189 returns early), the deep copy is wasteful. Consider passing the original block and only copying if rules exist.
| from draftjs_exporter.error import ConfigException | ||
| from draftjs_exporter.types import Block, ContentState, Entity, InlineStyleRange | ||
|
|
||
| FilterCallback: TypeAlias = Callable[[Any], Any] |
There was a problem hiding this comment.
The FilterCallback type alias uses Callable[[Any], Any] which is too permissive. Custom callbacks receive either Block, str (for inline_style), or Entity objects. Consider using a Union type or overloaded types for better static type checking.
| def import_markdown(self, markdown: str) -> ContentState: | ||
| """Parse Markdown and apply filter rules. | ||
|
|
||
| Parameters: | ||
| markdown: The Markdown text to import. | ||
|
|
||
| Returns: | ||
| The parsed, filtered ContentState. | ||
|
|
||
| Raises: | ||
| MarkdownParseError: If the input cannot be parsed. | ||
| """ |
There was a problem hiding this comment.
Docstring states MarkdownParseError is raised, but import_string raises ImportError for invalid parser paths. Either wrap the import or update the docstring to document all possible exceptions.
| """ | ||
| if config is None: | ||
| config = {} | ||
| parser_class = import_string(config.get("parser", DEFAULT_PARSER)) |
There was a problem hiding this comment.
The parser config option accepts arbitrary dotted paths passed to import_string. If user-controlled input flows here, it could enable arbitrary code execution. Consider documenting this risk or adding path validation for untrusted config sources.
| key = len(self.entity_map) | ||
| self.entity_map[str(key)] = { | ||
| "type": type_, | ||
| "mutability": mutability, | ||
| "data": data, | ||
| } | ||
| return key |
There was a problem hiding this comment.
Entity key type mismatch: add_entity stores entities with string keys (str(key)) in entity_map (typed dict[str, Entity]), but returns key as an int. Callers use the returned integer in entity_ranges (e.g., blocks.py line 197: {"offset": 0, "length": 1, "key": key}). When the HTML exporter looks up entities via entity_map.get(entity_key) with an integer key, the string-keyed lookup will return None, silently dropping entities. Either return str(key) instead of int, or store entities with integer keys and update the EntityMap type alias.
Suggestion:
| key = len(self.entity_map) | |
| self.entity_map[str(key)] = { | |
| "type": type_, | |
| "mutability": mutability, | |
| "data": data, | |
| } | |
| return key | |
| key = len(self.entity_map) | |
| self.entity_map[str(key)] = { | |
| "type": type_, | |
| "mutability": mutability, | |
| "data": data, | |
| } | |
| return str(key) |
| } | ||
| ) | ||
| cs = parser.parse("[label](wagtail://page?id=3)") | ||
| self.assertEqual(cs["entityMap"]["0"]["data"], {"id": 3}) |
There was a problem hiding this comment.
Test accesses entityMap with string key '0', but builder.add_entity returns int. The test passes only because the builder stores entities under str(key) ('0') instead of int(key) (0). This masks the underlying bug in builder.py where the return type annotation promises int (per entityRanges contract) but the map uses string keys. If builder.py is fixed to store integer keys, this assertion would raise KeyError and need to be updated to cs['entityMap'][0]. This is a test-level symptom of the builder.py bug; fixing builder.py will require updating this test accordingly.
Suggestion:
| self.assertEqual(cs["entityMap"]["0"]["data"], {"id": 3}) | |
| self.assertEqual(cs["entityMap"][0]["data"], {"id": 3}) |
| referenced = {str(r["key"]) for b in cs["blocks"] for r in b["entityRanges"]} | ||
| self.assertEqual(set(cs["entityMap"].keys()), referenced) |
There was a problem hiding this comment.
This test's assertion explicitly converts entity range keys to strings ({str(r["key"]) for ...}) before comparing against entityMap.keys(). This works around the builder.py inconsistency (where entityMap uses string keys while entityRanges use integers) rather than exposing it. Once builder.py is fixed to use integer keys, this str() conversion should be removed and the assertion changed to self.assertEqual(set(cs['entityMap'].keys()), referenced) with referenced = {r['key'] for ...}.
Suggestion:
| referenced = {str(r["key"]) for b in cs["blocks"] for r in b["entityRanges"]} | |
| self.assertEqual(set(cs["entityMap"].keys()), referenced) | |
| referenced = {r["key"] for b in cs["blocks"] for r in b["entityRanges"]} | |
| self.assertEqual(set(cs["entityMap"].keys()), referenced) |
| def test_links_disabled(self): | ||
| parser, builder = parse_with_builder(links=False) | ||
| text, _, entities = parser.parse("[a](/b)") | ||
| self.assertEqual(text, "[a](/b)") | ||
| self.assertEqual(entities, []) |
There was a problem hiding this comment.
The variable builder is created and assigned but never used in this test. Either remove it, or use it to verify the entity was not created in the entity map.
1 participant
Summary
Adds an experimental Markdown importer, complementing the existing Markdown exporter:
MarkdownImporterconverts Markdown text into a Draft.js ContentState. Dependency-free, no new install requirements.Two-phase architecture (design spec:
docs/superpowers/specs/2025-07-21-markdown-importer-design.md):MarkdownParser— hand-written parser covering the CommonMark core: paragraphs, ATX headings, blockquotes, fenced code, thematic breaks, nested lists with depth tracking, bold/italic/code, links, images, hard breaks. Guarantees structural integrity: every input produces a valid ContentState or raisesMarkdownParseError(with line numbers).ContentStateFilter— declarative content policy on the parsed result (remove/keep/demote/ callables), reusable on any ContentState. E.g. demoting level-1 headings:{"type": "block", "match": "header-one", "action": "demote"}.Configurable entity resolution for links and images: resolver chains route URLs to typed entities, with a shipped
scheme_resolverhelper for internal URL schemes (wagtail://image?id=10&format=left→ fully-populated IMAGE entity; plain/media/...jpg→ defaultsrc/alt). A configurableinline_html_styleswhitelist imports paired tags like<sup>as inline styles; all other HTML passes through as literal text, so there is no markup injection surface.The parser is referenced by dotted path in config, so an alternative engine (e.g. backed by a full CommonMark parser) can be swapped in later.
Supersedes the earlier first-draft approach on the
markdown-importerbranch with the approved class-based API (MarkdownImporter(config).import_markdown(md)).Test evidence
just test— 587 passed (unit, integration, snapshot, property-based)just test-coverage— 100% on all new modules, 100% project-widejust test-compatibility— 587 passed on Python 3.10 with pinned old depsjust lint— ruff check, ruff format, mypy, ty all cleanjust docs-build— cleanSnapshot coverage reuses the existing
test_exports.jsonfixtures: each fixture's recorded Markdown output is imported and compared against its ContentState, with explicit"import"overrides documenting known information loss (e.g. single-tilde strikethrough, entity metadata Markdown cannot carry).Included
markdown_parser,contentstate_filter,markdown_importerMarkdownImporter,MarkdownParser,ContentStateFilter,scheme_resolver,ImporterConfig,ParserConfig,FilterRule,EntityResolver,EntityResolution,MarkdownParseErrordocs/markdown-importer.md), nav entry, changelog entry, example.py import demo, agent skill update