fix: Force Unix style path for image URI fix - #641
Conversation
|
✅ DCO Check Passed Thanks @clach04, all your commits are properly signed off. 🎉 |
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Ensure image URI uses Unix style path for compatibility. Signed-off-by: clach04 <clach04@gmail.com>
|
@clach04 Thanks for this contribution. Can you please ensure the CI tests pass, by running |
Thank you @cau-git for this hint 🙏 Good news / bad news. This helped id formatting issue, which is resolved :-) bad news: Out of box, the headrev MyPytests failed (related to missing huggingface_hub module). I'm not likely to have time to dig into this but it looks like some dependency rules are quite 100% with the toml/prek config. But it worked well enough to catch the formatting issue :-) Is it possible to have the mergebot make the recommendation you did (by hand) for future PR reviews? It's super useful for people like myself who are super new and do not know all the project best practices. |
I, clach04 <clach04@gmail.com>, hereby add my Signed-off-by to this commit: d7df702 Signed-off-by: clach04 <clach04@gmail.com>
There was a problem hiding this comment.
@clach04 Thanks for supporting Docling and suggesting this PR.
Unfortunately, I don't think this is the right approach. The field uri of ImageRef class is typed as Union[AnyUrl, Path]. This PR changed the assignment to Path(...).as_posix() which returns a str, incompatible with the declared type. This is why the code checks fail.
An alternative approach could be to change the uri annotation to Union[AnyUrl, PurePosixPath] and add a pydantic field validator that converts any plain Path input into PurePosixPath. However, this change would not be backwards compatible and could impact existing client applications (e.g., isinstance(..., Path) breakage).
A safer choice is to keep Union[AnyUrl, Path] and fix the two serializer call sites (markdown, HTML), to call .as_posix() when rendering a Path uri, so the output always uses forward slashes, as proposed by #663
But in general .as_posix() would still have several limitations:
AnyUrldoesn't have it. Theurifield isUnion[AnyUrl, Path]and theREFERENCEDmode branch does not pre-filter by type, so calling.as_posix()on anAnyUrlwould raiseAttributeErrorat runtime.- It doesn't percent-encode anything.
as_posix()only replaces\with/on Windows paths. It leaves spaces, parentheses, #, ? and all other Markdown-breaking characters completely untouched. A path likeimg (1).pngor/home/a b/img.pngwould still break the Markdown inline link. - It doesn't handle absolute Windows paths or UNC shares specially.
PureWindowsPath("C:/…").as_posix()returns"C:/…"andurlsplitwould then parse C as a URL scheme.PureWindowsPath("//server/…").as_posix()returns"//server/…", which looks like a scheme-relative URL. Neither gets thefile://prefix it needs to be unambiguous.
I think that the PR #698 goes more in the right direction by defining a custom _escape_uri_path to address several edge cases. It should just need to be extended to the HTML serializer.
I would then suggest that we close this PR and we follow up with #698 to resolve docling-project/docling#3617
Ensure image URI uses Unix style path for compatibility.
Resolves docling-project/docling#3617