fix(models): don't crash to_markdown() on a malformed heading level - #178
Closed
afonsojanu wants to merge 1 commit into
Closed
fix(models): don't crash to_markdown() on a malformed heading level#178afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
_item_to_markdown() sliced heading['level'][1:] and int()'d it unconditionally, so any value not shaped like h1-h6 (e.g. 'title', '', None) raised ValueError/TypeError instead of degrading gracefully. TextExtractor only ever emits h1-h6, but ScrapeResult also accepts user-supplied data, where one malformed heading crashed the entire to_markdown() call. Add _heading_level(), which parses defensively and falls back to h2 (clamping in-range values like h9/h0 to 1-6), and route the heading loop through it. Fixes mldsveda#176
Collaborator
|
Thanks for this @afonsojanu , and it's a clean fix, the _heading_level helper is nice. Two PRs landed for #176 within a minute of each other though, and I went with #177 since it was green and conflict-free. No reflection on this one. Really appreciate you jumping on it, especially right after #172, if you want another, #166 (bound the on-disk cache) is open and meatier. |
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.
Fixes #176
Summary
ScrapeResult._item_to_markdown()slicedheading["level"][1:]andint()'d it unconditionally (int(heading.get("level", "h2")[1:])), so any level not shaped likeh1-h6raisedValueError("title","") orTypeError(None) instead of degrading gracefully.TextExtractoronly ever emitsh1-h6, butScrapeResultalso accepts user-supplieddata(a documented, supported use), where one malformed heading crashed the entireto_markdown()call rather than just that heading.Fix
Added
_heading_level(), which parses the level defensively — falls back toh2for anything not shapedh<digits>, and clamps in-range-shaped-but-out-of-bounds values (h9,h0) to 1-6 — and routed the heading loop through it, per the fix sketched in the issue.Testing
test_to_markdown_handles_a_malformed_heading_level— parametrized over"title","",None,"h9","h0": each renders without raising, with a heading line carrying 1-6#s.test_to_markdown_clamps_out_of_range_heading_levels— pins the exact clamped output forh1,h6,h9,h0.git stashonmodels.py, re-ran): 7 of 9 new cases failed with the exactValueError/TypeError/assertion mismatches the issue describes.mypy src/pyscrappy/core/models.pyreports 4 pre-existing errors (missingpandas/yamlstubs, twono-any-returnfindings on unrelated lines) — none touch_heading_levelor the lines I changed; unrelated to this fix.