refactor: simplify code for unicode extra field and improve error message#724
refactor: simplify code for unicode extra field and improve error message#724Its-Just-Nans merged 3 commits intomasterfrom
Conversation
Its-Just-Nans
commented
Mar 15, 2026
- Improve the code of the crc32 calculation (direct hash)
- Improve the error message of the crc32 calculation
- Add tests to the crc32 calculation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the handling of Unicode extra fields by streamlining the CRC32 checksum verification process. The changes aim to make the code more concise, provide clearer error feedback when checksums do not match, and improve the overall robustness through the addition of dedicated unit tests. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Summary
This PR simplifies the CRC32 calculation and improves error messages, which aligns well with the PR's stated goals. The code changes are good, but there's a critical defect in the test suite that needs to be fixed before merging.
Critical Issue
- The test
unicode_extra_field_crc32_incorrecthas inverted logic - it expects success when it should expect failure, which means it's not actually testing the error case
Changes Reviewed
- ✅ Simplified CRC32 calculation using direct
hash()call - ✅ Improved error message with actual vs expected values
- ❌ Test for incorrect CRC32 has wrong assertion
Please fix the test assertion before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
Code Review
The pull request simplifies the CRC32 calculation and improves the error message, which are good changes. However, the newly added tests are incorrect. They use an invalid version for the Unicode extra field, an incorrect data size, and in one case, a wrong assertion. I've provided suggestions to fix these tests.