Repository navigation
conformance: manifest-reference fixtures read and write as UTF-8 (#487 follow-up) - #489
babyblueviper1 wants to merge 2 commits into
Conversation
…gentrust-io#487 review) The loader used a bare read_text(), so on Windows the default cp1252 decoding broke test_fixtures_are_reproducible (the fixtures carry a section sign). The generator wrote with the default encoding too. Both are UTF-8 now; the six fixtures regenerate byte-identically. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: babyblueviper1 <babyblueviperbusiness@gmail.com>
|
🔴 Contributor Check: HIGH
Automated check by AgenTrust Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
There was a problem hiding this comment.
the fix is correct and necessary,the fixtures contain § (U+00A7) in the why fields, so the bare read_text() was indeed broken on Windows (cp1252), explicit encoding="utf-8" on both the loader and the generator is the right change for that
However, the PR’s claim that “the six fixtures regenerate byte-identically” is not actually guaranteed on Windows.
also it will good to strengthen test_fixtures_are_reproducible() to compare the actual generated bytes against the committed fixtures (not just the parsed objects)
| (HERE / f"{v['name']}.json").write_text(json.dumps(v, indent=2, ensure_ascii=False) + "\n") | ||
| (HERE / f"{v['name']}.json").write_text( | ||
| json.dumps(v, indent=2, ensure_ascii=False) + "\n", encoding="utf-8" | ||
| ) |
There was a problem hiding this comment.
Path.write_text defaults to newline=None and when newline=None and \n is translated to os.linesep and on windows this produces \r\n
…-identically Rajnish Tiwari's review on agentrust-io#489 pointed out that write_text(encoding="utf-8") alone still lets Path.write_text translate "\n" to os.linesep (CRLF on Windows), so the six fixtures do not actually regenerate byte-identically cross-platform as the PR claimed, even though test_fixtures_are_reproducible wouldn't catch it (it compares parsed objects, not bytes). newline="" disables that translation; regenerating now produces no diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: babyblueviper1 <babyblueviperbusiness@gmail.com>
|
Good catch, and correct: Fixed in 93f1eb7: |
There was a problem hiding this comment.
Reviewed the commit, 93f1eb7, the encoding="utf-8" + newline="" fixes both the Windows decoding issue and the byte-identity guarantee, in the codebase mostly generators uses newline="\n", however newline="" behaves the same
Optional nit still stands : test_fixtures_are_reproducible() still only compares parsed objects, comparing the actual generated bytes can make the guarantee stronger
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Ran at 93f1eb7 on Windows: 12 of 12 pass and the six fixtures regenerate byte-identically; main's loader fails test_fixtures_are_reproducible on the same machine.
This branch has not been deployed
3 participants
Follow-up to @imran-siddique's non-blocking note on #487.
VECTORSloaded with a bareread_text(), so on Windows the default cp1252 decoding breakstest_fixtures_are_reproducible, since the fixtures contain a section sign. The generator also wrote with the default encoding, so it now writes UTF-8 as well. The six fixtures regenerate byte-identically.pytest tests/test_manifest_ref_draft_vectors.pygives 12 passed, andruff checkandruff format --checkare clean. DCO signed off.Type of change
Tests for a fix
Not a code or schema fix: test-fixture I/O only.