fix(markdown): escape referenced figures' filepaths and ensure cross-platform path handling (Windows / POSIX) - #698
Open
the-zucc wants to merge 5 commits into
Open
fix(markdown): escape referenced figures' filepaths and ensure cross-platform path handling (Windows / POSIX)#698the-zucc wants to merge 5 commits into
the-zucc wants to merge 5 commits into
Conversation
Contributor
|
✅ DCO Check Passed Thanks @the-zucc, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🔴 1 of 2 protections blocking · waiting on 👀 reviews
🔴 Require two reviewer for test updatesWaiting for
This rule is failing.When test data is updated, we require two reviewers
Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
I, Laurier Lavoie-Giasson <laurier@laurier.dev>, hereby add my Signed-off-by to this commit: 513a8d5 Signed-off-by: Laurier Lavoie-Giasson <laurier@laurier.dev>
the-zucc
changed the title
added 2 commits
July 30, 2026 00:27
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
the-zucc
changed the title
ceberam
requested changes
Aug 14, 2026
Comment on lines
+726
to
+729
| _URI_KEEP_CHARS: str = "/%:@+,;=~$!&'*" | ||
|
|
||
| # Matches the drive prefix of an absolute Windows path, e.g. ``C:/``. | ||
| _WINDOWS_DRIVE_RE: re.Pattern = re.compile(r"[A-Za-z]:/") |
Member
There was a problem hiding this comment.
_URI_KEEP_CHARS and _WINDOWS_DRIVE_RE are defined inside the method body and both constants are reconstructed on every call. You can move them out, at class level. You can also annotate them as Final.
|
|
||
|
|
||
| # A link destination to encode, paired with its expected encoding. | ||
| _EscapeCase = tuple[Union[AnyUrl, PurePath], str] |
Member
There was a problem hiding this comment.
Minor comment: use TypeAlias as per PEP 613.
Comment on lines
+722
to
+725
| # Characters that survive percent-encoding in a link destination. | ||
| # The RFC 3986 reserved characters that carry meaning in a URI, plus ``%`` so that an | ||
| # already-encoded destination is not encoded a second time. Whitespace and parentheses | ||
| # are deliberately absent: they would end a Markdown inline link. |
Member
There was a problem hiding this comment.
Could you please:
- tighten the prose to reduce verbosity
- instead of inline comments, add this information as docstrings for these constants in google style
Comment on lines
+712
to
+713
| Known limitation: behavior is unknown if value is a POSIX filename, containing | ||
| an actual backslash in the filename. |
Member
There was a problem hiding this comment.
The behavior is wrong but deterministic: the backslash is misinterpreted as a separator. But this is harmless in practice. Please, accept this suggestion:
Suggested change
| Known limitation: behavior is unknown if value is a POSIX filename, containing | |
| an actual backslash in the filename. | |
| Known limitation: a backslash in a `PosixPath` string is ambiguous. | |
| It may be a Windows separator surviving a JSON round-trip (correct to | |
| convert) or a literal filename character (where converting it to `/` | |
| would split one component into two). The two cases are indistinguishable | |
| from `str()`. In practice this is not a concern because `ImageRef.uri` | |
| is always populated from native filesystem operations, so a `PosixPath` | |
| can only carry a literal backslash if the caller explicitly constructed one. |
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
2 participants
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.
#182 caused a regression in which the behavior introduced in #122 was removed.
I'm therefore proposing that we re-add the change, slightly modified for both POSIX and Windows path support.
One edge-case identified is the obvious case where the destination path is a filename containing a backslash. I've mentioned the limitation in the docstring.
Partial fix for docling-project/docling#3617