Add experimental Markdown importer - #160
Conversation
|
🔍 OpenCodeReview found 13 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.
b77c524 to
52caa95
Compare
| Raises: | ||
| MarkdownParseError: If the input cannot be parsed. |
There was a problem hiding this comment.
The import_markdown docstring documents MarkdownParseError as raised, but the method could also propagate ConfigException from the filter (if invalid rules are provided) or ImportError from import_string. Consider documenting all exceptions or wrapping them for a consistent error interface.
| from draftjs_exporter.markdown_parser.resolvers import ( | ||
| EntityResolution as EntityResolution, | ||
| ) | ||
| from draftjs_exporter.markdown_parser.resolvers import ( | ||
| EntityResolver as EntityResolver, | ||
| ) | ||
| from draftjs_exporter.markdown_parser.resolvers import ( | ||
| scheme_resolver as scheme_resolver, | ||
| ) |
There was a problem hiding this comment.
Unnecessary self-aliasing import pattern. The X as X pattern aliases each name to itself, which is redundant and inconsistent with the simpler from module import X pattern used elsewhere in this file (lines 4-6).
| def __init__(self, config: ParserConfig | None = None) -> None: | ||
| """Initialize the parser with the given configuration. | ||
|
|
||
| Parameters: | ||
| config: Feature toggles and entity resolvers. Missing keys | ||
| use defaults that enable all constructs. | ||
| """ | ||
| self.config = config if config is not None else ParserConfig() |
There was a problem hiding this comment.
ParserConfig TypedDict lacks runtime validation. The docstring documents expected types (e.g., 'headings: bool'), but these are not enforced at runtime. If a caller passes invalid values like 'headings': None or 'headings': 'true', the error will manifest as confusing AttributeError or unexpected behavior in BlockParser/InlineParser rather than failing fast with a clear TypeError. Consider validating config values at the start of __init__ or using a validation pattern consistent with the project's existing ConfigException usage.
| def parse(self, markdown: str) -> ContentState: | ||
| """Parse Markdown source into a ContentState. | ||
|
|
||
| Parameters: | ||
| markdown: The Markdown text to parse. | ||
|
|
||
| Returns: | ||
| A structurally valid Draft.js ContentState. | ||
|
|
||
| Raises: | ||
| TypeError: If ``markdown`` is not a string. | ||
| MarkdownParseError: If an entity resolver fails. | ||
| """ | ||
| if not isinstance(markdown, str): | ||
| raise TypeError(f"Expected str, got {type(markdown).__name__}") |
There was a problem hiding this comment.
The docstring documents 'MarkdownParseError' for entity resolver failures, but the code only checks for TypeError for the input validation. Entity resolver errors are handled by the delegated InlineParser which correctly wraps unexpected exceptions (see inline.py:149-152), so the public interface is correct. However, the docstring could be improved by documenting all possible error paths: TypeError for non-string input, and MarkdownParseError for entity resolver failures or excessive nesting.
| def test_invalid_rule_type(self): | ||
| with self.assertRaises(ConfigException): | ||
| ContentStateFilter([{"type": "nope", "match": "x", "action": "keep"}]) # type: ignore[typeddict-item] # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Redundant type ignore comments. These lines have two separate # type: ignore directives when one would suffice. Consider using # type: ignore[typeddict-item, invalid-argument-type] to consolidate them.
Suggestion:
| def test_invalid_rule_type(self): | |
| with self.assertRaises(ConfigException): | |
| ContentStateFilter([{"type": "nope", "match": "x", "action": "keep"}]) # type: ignore[typeddict-item] # ty: ignore[invalid-argument-type] | |
| def test_invalid_rule_type(self): | |
| with self.assertRaises(ConfigException): | |
| ContentStateFilter([{"type": "nope", "match": "x", "action": "keep"}]) # type: ignore[typeddict-item, invalid-argument-type] |
| def scheme_resolver( | ||
| scheme: str, | ||
| type_map: dict[str, str], | ||
| coerce: dict[str, Callable[[str], Any]] | None = None, |
There was a problem hiding this comment.
The coerce parameter uses Callable[[str], Any] for the converter return type. While acceptable, this could use a type variable for consistency if the expected output types are known (e.g., JSONValue). For now, Any is acceptable given the flexibility needed.
| ) | ||
| block = builder.build()["blocks"][0] | ||
| self.assertEqual(block["depth"], 2) | ||
| self.assertEqual(block["entityRanges"][0]["key"], 0) |
There was a problem hiding this comment.
The test passes key (an int) in entity_ranges, and the assertion only checks block['entityRanges'][0]['key'] == 0. However, there's no verification that the entityRanges key (int) correctly maps to the entityMap key (string). If a caller accidentally passes a string key to entityRanges, it won't work correctly. Consider adding an integration test that verifies entity references resolve correctly end-to-end.
Suggestion:
| self.assertEqual(block["entityRanges"][0]["key"], 0) | |
| # Add test: verify the full round-trip - entity registered, referenced in block, | |
| # and the exported JSON matches expected structure from test_imports.json |
| def test_depth_guard_rejects_excessive_recursion(self): | ||
| from draftjs_exporter.error import MarkdownParseError | ||
| from draftjs_exporter.markdown_parser.inline import MAX_INLINE_DEPTH |
There was a problem hiding this comment.
Inline imports of MarkdownParseError and MAX_INLINE_DEPTH inside the test method is consistent with the pattern used in other test files in this module (e.g., test_inline.py line 366, test_blocks.py line 271). No action needed.
| self.assertEqual(styles, []) | ||
|
|
||
|
|
||
| class TestNestingDepth(unittest.TestCase): |
There was a problem hiding this comment.
While TestNestingDepth tests depth-limiting behavior (a different concern from inline HTML whitelist handling), the file organization is acceptable. Both test classes share the same SUP_SUB constant and make_parser helper, and splitting them would introduce unnecessary duplication. The current structure is reasonable for test organization.
|
|
||
| def test_non_string_input_raises_type_error(self): | ||
| with self.assertRaises(TypeError): | ||
| MarkdownParser().parse(None) # type: ignore[arg-type] # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Minor typo in type ignore comment: ty should be type. Also, the # type: ignore[arg-type] on the same line makes the second ignore redundant — a single # type: ignore[arg-type] suffices.
| __slots__ = ("line", "message") | ||
|
|
||
| def __init__(self, message: str, line: int | None = None) -> None: | ||
| """Initialize the error with a message and optional line number. | ||
|
|
||
| Parameters: | ||
| message: Human-readable description of the failure. | ||
| line: 1-based source line number, if known. | ||
| """ | ||
| self.message = message | ||
| self.line = line | ||
| super().__init__(f"line {line}: {message}" if line is not None else message) |
There was a problem hiding this comment.
The message attribute stores a duplicate of the exception's string argument. Consider storing only the line attribute and accessing the message via super().__str__() or the exception's args attribute to avoid redundancy and potential drift.
Suggestion:
| __slots__ = ("line", "message") | |
| def __init__(self, message: str, line: int | None = None) -> None: | |
| """Initialize the error with a message and optional line number. | |
| Parameters: | |
| message: Human-readable description of the failure. | |
| line: 1-based source line number, if known. | |
| """ | |
| self.message = message | |
| self.line = line | |
| super().__init__(f"line {line}: {message}" if line is not None else message) | |
| __slots__ = ("line",) | |
| def __init__(self, message: str, line: int | None = None) -> None: | |
| """Initialize the error with a message and optional line number. | |
| Parameters: | |
| message: Human-readable description of the failure. | |
| line: 1-based source line number, if known. | |
| """ | |
| self.line = line | |
| super().__init__(f"line {line}: {message}" if line is not None else message) | |
| @property | |
| def message(self) -> str: | |
| """Return the error message.""" | |
| return self.args[0] if self.args else "" |
| Raises: | ||
| TypeError: If ``markdown`` is not a string. | ||
| MarkdownParseError: If an entity resolver fails. |
There was a problem hiding this comment.
The Raises: docstring documents MarkdownParseError but the exception class is not imported in this module. Callers relying on IDE autocompletion or static analysis will not find the import. Either import it here for completeness or rely on the fact that it's raised transitively through BlockParser.parse() (in which case the docstring should note this).
Suggestion:
| Raises: | |
| TypeError: If ``markdown`` is not a string. | |
| MarkdownParseError: If an entity resolver fails. | |
| Raises: | |
| TypeError: If ``markdown`` is not a string. | |
| MarkdownParseError: If an entity resolver fails (raised by BlockParser.parse()). |
| """ | ||
| if config is None: | ||
| config = {} | ||
| parser_class = import_string(config.get("parser", DEFAULT_PARSER)) |
There was a problem hiding this comment.
The import_string function uses import_module which allows importing any module in Python's search path, not just draftjs_exporter internals. A malicious config value like "os.system" or "subprocess.Popen" could be passed to execute arbitrary code. Consider restricting the parser path to the draftjs_exporter namespace unless third-party parsers are explicitly supported. For example: validate that the dotted path starts with a known prefix like draftjs_exporter. or a configured allowlist.
| FilterCallback: TypeAlias = Callable[[Any], Any] | ||
| """Custom rule action: receives the matched object, returns a replacement or None.""" |
There was a problem hiding this comment.
The FilterCallback type alias uses Callable[[Any], Any] which is overly loose. The code actually constrains valid return types in _run_actions() validation, but the type annotation doesn't reflect this. Consider using a more precise type or documenting the actual constraints in the docstring.
Suggestion:
| FilterCallback: TypeAlias = Callable[[Any], Any] | |
| """Custom rule action: receives the matched object, returns a replacement or None.""" | |
| # Input is dict (Block or Entity) or str (inline style name) | |
| FilterCallback: TypeAlias = Callable[[dict[str, Any] | str], dict[str, Any] | str | None] | |
| """Custom rule action: receives the matched object, returns a replacement or None. | |
| Input types: | |
| - block/entity rules: dict with 'type' key and other properties | |
| - inline_style rules: str (style name) | |
| Return types: | |
| - None to remove the object | |
| - dict for block/entity replacements | |
| - str for inline style name replacements | |
| """ |
| styles_by_run = { | ||
| 1: [INLINE_STYLES.ITALIC], | ||
| 2: [INLINE_STYLES.BOLD], | ||
| 3: [INLINE_STYLES.BOLD, INLINE_STYLES.ITALIC], | ||
| } | ||
| for style in styles_by_run[run]: |
There was a problem hiding this comment.
The styles_by_run dictionary is created on every invocation of _parse_emphasis(). While keys 1, 2, 3 are currently guaranteed by the preceding run > 3 check, this pattern is fragile: modifying the early exit condition could introduce a silent KeyError. Additionally, run == 3 (triple emphasis producing both BOLD and ITALIC) has no test coverage.
Suggestion:
| styles_by_run = { | |
| 1: [INLINE_STYLES.ITALIC], | |
| 2: [INLINE_STYLES.BOLD], | |
| 3: [INLINE_STYLES.BOLD, INLINE_STYLES.ITALIC], | |
| } | |
| for style in styles_by_run[run]: | |
| # Option 1: Move to module-level constant | |
| _EMPHASIS_STYLES: dict[int, list[str]] = { | |
| 1: [INLINE_STYLES.ITALIC], | |
| 2: [INLINE_STYLES.BOLD], | |
| 3: [INLINE_STYLES.BOLD, INLINE_STYLES.ITALIC], | |
| } | |
| # Option 2: Use direct conditional logic | |
| if run == 1: | |
| style_list = [INLINE_STYLES.ITALIC] | |
| elif run == 2: | |
| style_list = [INLINE_STYLES.BOLD] | |
| else: # run == 3 | |
| style_list = [INLINE_STYLES.BOLD, INLINE_STYLES.ITALIC] | |
| for style in style_list: |
| def test_parse_error_propagates(self): | ||
| def bad(url, label): | ||
| raise RuntimeError("boom") | ||
|
|
||
| importer = MarkdownImporter({"parser_config": {"link_resolvers": [bad]}}) | ||
| with self.assertRaises(MarkdownParseError): | ||
| importer.import_markdown("[a](/b)") |
There was a problem hiding this comment.
The test test_parse_error_propagates verifies that exceptions from custom resolvers are wrapped as MarkdownParseError. The implementation in inline.py:149-152 uses except Exception to catch and wrap resolver errors. This is correct behavior that preserves the exception chain with from err.
Suggestion:
| def test_parse_error_propagates(self): | |
| def bad(url, label): | |
| raise RuntimeError("boom") | |
| importer = MarkdownImporter({"parser_config": {"link_resolvers": [bad]}}) | |
| with self.assertRaises(MarkdownParseError): | |
| importer.import_markdown("[a](/b)") | |
| The implementation correctly: | |
| 1. Re-raises MarkdownParseError without wrapping (line 146-148) | |
| 2. Catches other exceptions and wraps them with `from err` to preserve the cause | |
| 3. Does not use a bare `except:` clause | |
| No action needed. |
| def test_wagtail_style_end_to_end(self): | ||
| importer = MarkdownImporter( | ||
| { | ||
| "parser_config": { | ||
| "image_resolvers": [ | ||
| scheme_resolver( | ||
| "wagtail", | ||
| {"image": "IMAGE"}, | ||
| coerce={"id": int}, | ||
| label_key="alt", | ||
| mutability="IMMUTABLE", | ||
| ) | ||
| ] | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
The test_wagtail_style_end_to_end test verifies successful coercion with valid input. However, if the coerce function (e.g., int) raises an exception for invalid input (like "abc"), the error would propagate uncaught (see resolvers.py:124). This is a missing edge case in the resolver tests, not an issue with this test file.
Suggestion:
| def test_wagtail_style_end_to_end(self): | |
| importer = MarkdownImporter( | |
| { | |
| "parser_config": { | |
| "image_resolvers": [ | |
| scheme_resolver( | |
| "wagtail", | |
| {"image": "IMAGE"}, | |
| coerce={"id": int}, | |
| label_key="alt", | |
| mutability="IMMUTABLE", | |
| ) | |
| ] | |
| } | |
| } | |
| ) | |
| Consider adding a test case for coercion failure to `tests/markdown_parser/test_resolvers.py`: | |
| ```python | |
| def test_scheme_resolver_coerce_failure(self): | |
| resolver = scheme_resolver("wagtail", {"image": "IMAGE"}, coerce={"id": int}) | |
| with self.assertRaises(ValueError): | |
| resolver("wagtail://image?id=abc", "alt") | |
| ``` | |
| This ensures users understand that invalid coercion values will raise errors rather than being silently ignored. |
|
|
||
| def test_non_string_input_raises_type_error(self): | ||
| with self.assertRaises(TypeError): | ||
| MarkdownParser().parse(None) # type: ignore[arg-type] # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Typo in type ignore comment: 'ty: ignore' should be 'type: ignore'. While this is a test file with intentionally suppressive comments, the malformed comment could confuse linters or tooling.
| def test_demote_headings(self): | ||
| cs = cs_with_blocks( | ||
| make_block("header-one"), | ||
| make_block("header-three"), | ||
| ) |
There was a problem hiding this comment.
Test name test_demote_headings (plural) implies it tests demoting multiple heading levels (e.g., header-one→header-two and header-three→header-four). However, the test only applies a demote rule to header-one, leaving header-three unchanged. This creates a misleading test name. Consider renaming to test_demote_single_header or adding a rule for header-three to match the plural intent.
| new_map = {} | ||
| for old_key, new_key in key_map.items(): | ||
| if old_key in entity_map: | ||
| new_map[new_key] = entity_map[old_key] | ||
| return {"blocks": blocks, "entityMap": new_map} |
There was a problem hiding this comment.
The normalize() function's entity remapping silently drops entities that exist in entityMap but are never referenced by any entityRanges. This could mask importer bugs where entities are created but not properly linked, or cause false test failures if fixture data has unreferenced entities. Consider either preserving orphaned entities or adding an assertion to catch this case explicitly.
Accept the full CommonMark backslash-escapable punctuation set so the
importer inverts the exporter's text escaping (including line-start '='
and '~' escapes). Unescape '\\', '\\(', '\\)' in link and image
destinations, mirroring escape_link_destination, so URLs with parentheses
round-trip. Size code span delimiters by matching equal-length backtick
runs and apply CommonMark space-padding normalization, mirroring
code_span_delimiters.
Add direct-import fixtures for escaped line-start characters, ordered list markers, metacharacters, and link destinations with escaped parentheses, plus sized code spans. Add an escaping round-trip test class that asserts the exporter's escaped Markdown imports back as the original text.
Document backslash-escape inversion, link destination unescaping, and sized code span parsing in the importer docs and design spec. Replace the now-handled gaps with the remaining underscore-flanking limitation, explaining why it requires an exporter-side change. Correct the stale 'no HTML escaping in text' note in the skill once main added escaping.
Note that import_markdown can raise ConfigException from filter callbacks at runtime, not just MarkdownParseError. Document that repeated query keys in scheme_resolver URLs keep the last value.
5a444f6 to
47e0a13
Compare
Superpowers design docsThe Markdown importer was developed with a design spec and implementation plan under
These documents reflect the original design intent; the shipped implementation may differ in places (notably the escaping round-trip work added after the exporter's text-escaping upgrade on |
| class MarkdownParseError(ExporterException): | ||
| """Raised when Markdown input cannot be parsed. | ||
|
|
||
| Carries an optional 1-based line number pointing at the source of | ||
| the failure. | ||
| """ | ||
|
|
||
| __slots__ = ("line", "message") |
There was a problem hiding this comment.
The line number attribute could be documented in the class docstring's attributes section for completeness. When a class defines __slots__, it's helpful to list them explicitly for users who may want to catch specific attributes.
| """ | ||
| if config is None: | ||
| config = {} | ||
| parser_class = import_string(config.get("parser", DEFAULT_PARSER)) |
There was a problem hiding this comment.
The import_string function dynamically loads the parser class from a user-provided dotted path. If this configuration can be controlled by external input (e.g., web requests), an attacker could potentially execute arbitrary code by providing a malicious path. Consider validating the path against an allowlist of trusted parsers, or document this as an internal-only configuration.
| ConfigException: If a filter callback returns an invalid value | ||
| during filtering. | ||
| """ | ||
| return self.filter.apply(self.parser.parse(markdown)) |
There was a problem hiding this comment.
The parse() method can raise TypeError when given non-string input, but this exception is not documented in import_markdown's docstring. Consider adding TypeError to the Raises section for completeness.
| FilterCallback: TypeAlias = Callable[[Any], Any] | ||
| """Custom rule action: receives the matched object, returns a replacement or None.""" |
There was a problem hiding this comment.
The FilterCallback TypeAlias uses Any for both input and output types (Callable[[Any], Any]), which is overly permissive. While the callback receives different types (Block, str, or Entity) depending on the rule kind, the current typing doesn't reflect this. Consider documenting the expected input types in the docstring or using a more precise Union type.
| entity_map_out = {} | ||
| for key in used_keys: | ||
| entity = replacements.get(key, entity_map_in[key]) | ||
| entity_map_out[key] = entity |
There was a problem hiding this comment.
The replacements.get(key, entity_map_in[key]) fallback could raise KeyError if a key is in used_keys but missing from both replacements and entity_map_in. While this shouldn't occur with valid input, adding a defensive check would prevent crashes on malformed ContentState.
Suggestion:
| entity_map_out = {} | |
| for key in used_keys: | |
| entity = replacements.get(key, entity_map_in[key]) | |
| entity_map_out[key] = entity | |
| entity_map_out = {} | |
| for key in used_keys: | |
| if key in replacements: | |
| entity_map_out[key] = replacements[key] | |
| elif key in entity_map_in: | |
| entity_map_out[key] = entity_map_in[key] | |
| else: | |
| raise ConfigException(f"Entity key {key!r} referenced but not found") |
| Returns: | ||
| A resolver that defers (returns None) for non-matching URLs. | ||
| """ | ||
| converters = coerce if coerce is not None else {} |
There was a problem hiding this comment.
The scheme_resolver closure captures coerce by reference via converters. If the coerce dict is mutated after scheme_resolver() is called, the resolver's behavior may change unexpectedly. Consider copying the dict: converters = dict(coerce) if coerce else {}.
| mutability: Mutability = "MUTABLE", | ||
| ) -> EntityResolver: |
There was a problem hiding this comment.
The mutability parameter accepts the Mutability type alias (a Literal type), but there's no runtime validation. Invalid values like 'mutable' (lowercase) would pass type checking at static analysis but cause issues at runtime. Consider validating at the function entry point or documenting this expectation clearly.
| class TestRuleValidation(unittest.TestCase): | ||
| def test_invalid_rule_type(self): | ||
| with self.assertRaises(ConfigException): | ||
| ContentStateFilter([{"type": "nope", "match": "x", "action": "keep"}]) # type: ignore[typeddict-item] # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Typo in type ignore comment: 'ty: ignore' should be 'type: ignore'. This typo will cause the comment to be ineffective, and the type checker may still report warnings for the intentionally invalid inputs.
|
|
||
| def test_invalid_action(self): | ||
| with self.assertRaises(ConfigException): | ||
| ContentStateFilter([{"type": "block", "match": "x", "action": "nope"}]) # type: ignore[typeddict-item] # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Typo in type ignore comment: 'ty: ignore' should be 'type: ignore'. This typo will cause the comment to be ineffective, and the type checker may still report warnings for the intentionally invalid inputs.
| class TestRuleValidation(unittest.TestCase): | ||
| def test_invalid_rule_type(self): | ||
| with self.assertRaises(ConfigException): | ||
| ContentStateFilter([{"type": "nope", "match": "x", "action": "keep"}]) # type: ignore[typeddict-item] # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
Same typo: 'ty: ignore' should be 'type: ignore'.
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.
Escaping round-trip
Following the Markdown exporter's text-escaping upgrade on
main, the importer now inverts that escaping so escaped Markdown round-trips back to the original text:=and~).escape_link_destination, so URLs containing parentheses round-trip.code_span_delimiters, so code spans containing backticks round-trip.One remaining round-trip gap is documented rather than fixed: intraword underscore emphasis (identifiers like
foo_bar_bazvs. legitimate mid-word italic likefan_tastic_) is structurally ambiguous in the Markdown the exporter emits, so it requires an exporter-side change to resolve fully. See Known round-trip limitations.Test evidence
just test— 729 passed (unit, integration, snapshot, property-based)just lint— ruff check, ruff format, mypy, ty all clean; prettier clean on docs/JSONmain(which added Markdown text escaping): all green, including the escaping round-trip tests run against main's final escaped export snapshotsSnapshot 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). Direct escaping fixtures intests/test_imports.jsonand aTestEscapingRoundTripclass lock the exporter↔importer escaping contract.Included
markdown_parser,contentstate_filter,markdown_importerMarkdownImporter,MarkdownParser,ContentStateFilter,scheme_resolver,ImporterConfig,ParserConfig,FilterRule,EntityResolver,EntityResolution,MarkdownParseErrordocs/markdown-importer.md) with an Escaping section, nav entry, changelog entry, example.py import demo, agent skill update