feat: expose json.dumps kwargs in save_as_json - #725
Open
Abdur-Rafay-AR wants to merge 1 commit into
Open
Conversation
Signed-off-by: Abdur Rafay <abdurrafay.tech@gmail.com>
Contributor
|
✅ DCO Check Passed Thanks @Abdur-Rafay-AR, 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/
|
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.
Addresses docling-project/docling#3939 (filed on
docling, but the code lives here).Problem
DoclingDocument.save_as_jsonhardcodes thejson.dumpscall:ensure_asciiis never passed, so it takes the stdlib default ofTrueand every non-ASCII character is written as a\uXXXXescape. There is no way to override this from the public signature. The reporter hit it with Polish text, and the same thing happens with Urdu, CJK, and anything else outside ASCII. The current workaround is to reload every document and re-save it with thejsonmodule directly, which is not practical on large datasets.Change
Adds an optional
json_kwargs: Optional[dict[str, Any]] = Noneat the end of the signature, merged into thejson.dumpscall:json_kwargsis merged last so a caller can overrideindentwithout triggering aTypeErroron a duplicate keyword.Usage:
Backward compatibility
The change is purely additive. Every existing parameter keeps its position and default, and the new argument is the last one, so positional callers are unaffected. When
json_kwargsis omitted the output is byte for byte what it was before.The new test pins this explicitly. It asserts not only that
ensure_ascii=Falsewrites the literal characters, but also that the default path still emits the escaped form, so a future regression in either direction fails the suite:The existing
save_as_jsoncalls covered by the ground truth snapshots were deliberately left untouched.On
save_as_yamlsave_as_yamlhas the equivalent gap for a different reason: PyYAML'sallow_unicodedefaults toFalse, so theyaml.dumpcall escapes non-ASCII too. I left it out to keep this diff focused on the reported issue, but an equivalentyaml_kwargsis a small addition and I am happy to fold it into this PR if you want the two methods to stay consistent. Just say the word.Context
I commented on the issue two days ago proposing this approach and asking whether you wanted one method or both, and a passthrough dict or explicit named parameters. Rather than let it sit, I am opening the narrow version so there is something concrete to review. Happy to reshape it if you would prefer named parameters such as
ensure_asciiover a dict.Testing
ruff check,ruff format --check, andmypyare clean on the changed files. The compat projector and docs generation hooks report no changes, as expected, since a method parameter does not affect the exported schema.For the test suite, I compared the full run with and without the patch on the same environment and the set of failing tests is byte for byte identical, so this change introduces no regressions.
test/test_docling_doc.pygoes from 73 to 74 passing with the new test.One unrelated note, offered as an observation rather than something I touched: several tests in
test/test_docling_doc.pyfail on Windows because_verify_saved_outputcompares serialized artifact paths literally, so the generatedextracted_images\image_000does not match theextracted_images/image_000in the ground truth files. That failure cascades intotest_concatenateandtest_file_uri_allowed_with_env_var, which depend on a file the failing test would have written. Normalizing the separator before comparison would make the suite portable, but it is outside the scope of this PR.