Fix Zip converter forwarding kwargs to nested conversions - #2409
Conversation
RKS (rksharma-owg)
left a comment
RKS (rksharma-owg)
left a comment
There was a problem hiding this comment.
I reviewed this against the current main branch. The change is narrowly scoped and correctly preserves conversion options for files nested inside ZIP archives, including plugin/LLM-related kwargs.\n\nOne follow-up would make this regression-resistant: please add a focused test with a fake nested converter that records kwargs, asserting that options such as are forwarded. The existing ZIP coverage does not currently exercise this call contract.
RKS (rksharma-owg)
left a comment
RKS (rksharma-owg)
left a comment
There was a problem hiding this comment.
I reviewed this against the current main branch. The change is narrowly scoped and correctly preserves conversion options for files nested inside ZIP archives, including plugin and LLM-related kwargs.
One follow-up would make this regression-resistant: please add a focused test with a fake nested converter that records kwargs, asserting that options such as keep_data_uris are forwarded. The existing ZIP coverage does not currently exercise this call contract.
RKS (rksharma-owg)
left a comment
RKS (rksharma-owg)
left a comment
There was a problem hiding this comment.
The follow-up regression test now records kwargs received by the nested converter and verifies keep_data_uris=True is forwarded through ZIP conversion. I ran the focused test in the managed environment: 1 passed. Approved.
|
Hello, Thank you for reviewing and approving my PR! I'm glad I could contribute to the project. It was a pleasure helping out. |
|
You are welcome, and thanks for addressing the review feedback promptly. The added regression coverage makes the kwargs-forwarding contract clear. |
|
i am waiting for my name to be listed in microsoft Contributors list |
|
GitHub generally adds contributors after a pull request is merged and the repository contribution statistics refresh. This PR is still open, so it will not appear in the Microsoft/markitdown Contributors list yet. The commit is already attributed to your GitHub account (As9xm), so once the PR is merged, allow some time for the contributor list to update. |
|
man i am so happy, of course i will wait, i can't believe my code will be used in one of microsoft products, i am extremely happy |
There was a problem hiding this comment.
🟡 Changes recommended
Parent file_extension and url metadata can override child file metadata and select incorrect converters.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Forwards conversion options to files nested inside ZIP archives.
Changes:
- Passes nested conversion kwargs through
ZipConverter. - Adds a regression test verifying option forwarding.
- Review found parent ZIP metadata is incorrectly forwarded to child files.
File summaries
| File | Description |
|---|---|
_zip_converter.py |
Forwards kwargs to nested conversions. |
test_zip_kwargs.py |
Tests nested option forwarding. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| result = self._markitdown.convert_stream( | ||
| stream=z_file_stream, | ||
| stream_info=z_file_stream_info, | ||
| **kwargs, | ||
| ) |
RKS (rksharma-owg)
left a comment
RKS (rksharma-owg)
left a comment
There was a problem hiding this comment.
I reproduced the metadata regression Copilot identified on 870514f. My earlier approval covered option forwarding but missed this interaction with the legacy metadata kwargs.
Using the existing tests/test_files/test.docx fixture, standalone conversion produces 4,651 characters of Markdown. Putting that same file inside an in-memory ZIP and converting with StreamInfo(extension=".zip", url="https://example.test/archive.zip") produces 82,702 characters containing word/document.xml; the standalone DOCX output is absent. The current test_zip_kwargs.py still passes.
_convert injects the archive's file_extension and url into kwargs, and the nested convert_stream gives those precedence over the member's StreamInfo. Please filter those two archive metadata keys while forwarding the conversion options, and add regression coverage for member conversion with and without an archive URL. An in-memory trial of that filter preserved the DOCX output in both cases and kept the existing keep_data_uris test passing (Python 3.12.13, macOS).
No source changes were pushed. This needs a follow-up on this PR before my approval applies to the current behavior.
Adds a parametrized regression test converting a DOCX inside an in-memory ZIP, both with and without a url on the archive's StreamInfo, asserting the member's real conversion output is present and that it was not unpacked into its raw OOXML parts. Both cases fail against 870514f. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
soo, i have some fixes to do, and then it will be merged? |
|
SpongeBob (@As9xm) Nah, I'm about to merge it right now. Just small fixes for the CI, but I'll handle them on my end. Thanks for the contribution. |
|
afourney thank you so much, my first ever contribution to Microsoft Project, i know the Ai might done some small bugs, but i assure you next time i will try to reduce the bugs it produced, thanks again for helping me |
|
Thanks for the contribution! Don't worry about the AI feedback. It almost always has something to say about everything. :) |
ZipConvertercalledconvert_streamwithout**kwargs, so nested options likellm_client,keep_data_uris, and strict were lost for files inside zips.This change forwards
**kwargsto nested conversions.