python: fix BSUnit ctypes binding - #178
Conversation
Pass BSUnit as a ctypes union to bs_size_convert_to and add a conversion regression test. Also make the translation canary shell test POSIX-compatible.
📝 WalkthroughWalkthroughThe Python binding now uses a public ChangesUnit conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change corrects Python unit conversion binding behavior and updates related tests and shell compatibility; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/libbytesize_unittest.py (1)
368-369: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for decimal units.
This test covers only
MiBtoKiB. Add a1 MBtoKBassertion to verify the decimal-unit path throughSizeUnit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/libbytesize_unittest.py` around lines 368 - 369, Add a decimal-unit assertion alongside the existing SizeStruct.new_from_str test, converting “1 MB” to KB and expecting “1000”. Keep the current binary-unit MiB-to-KiB assertion unchanged to cover both SizeUnit paths.src/python/bytesize.py (1)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAddress the RUF012 warning on
SizeUnit._fields_.Ruff reports this list as a mutable class attribute. If Ruff is enforced, annotate
_fields_as aClassVaror use an immutable sequence accepted by the supportedctypesversions. Keep any suppression scoped to this declaration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/python/bytesize.py` around lines 84 - 88, Update the SizeUnit._fields_ declaration to address RUF012 by annotating it as a ClassVar or replacing the list with an immutable sequence supported by the project’s ctypes versions. Keep any lint suppression narrowly scoped to this declaration.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/python/bytesize.py`:
- Around line 84-88: Update the SizeUnit._fields_ declaration to address RUF012
by annotating it as a ClassVar or replacing the list with an immutable sequence
supported by the project’s ctypes versions. Keep any lint suppression narrowly
scoped to this declaration.
In `@tests/libbytesize_unittest.py`:
- Around line 368-369: Add a decimal-unit assertion alongside the existing
SizeStruct.new_from_str test, converting “1 MB” to KB and expecting “1000”. Keep
the current binary-unit MiB-to-KiB assertion unchanged to cover both SizeUnit
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a7dcd7f-8547-4720-8cab-f3c355cd7a25
📒 Files selected for processing (3)
src/python/bytesize.pytests/canary_tests.sh.intests/libbytesize_unittest.py
vojtechtrefny
left a comment
There was a problem hiding this comment.
Thank you, looks good to me in general, just few small changes.
| err = POINTER(SizeErrorStruct)() | ||
| ret = c_bytesize.bs_size_convert_to(self, unit, byref(err)) | ||
| u = SizeUnit() | ||
| u.bunit = unit |
There was a problem hiding this comment.
This works, but I think it would be better to set the correct member based on the unit. So dunit for MB/21 or larger and bunit else.
| x = SizeStruct.new_from_str("1 KiB") | ||
| x.convert_to(KiB) | ||
| x = SizeStruct.new_from_str("1 MiB") | ||
| self.assertEqual(x.convert_to(KiB), "1024") |
There was a problem hiding this comment.
Please also add a check with a decimal unit.
Pass BSUnit as a ctypes union to bs_size_convert_to and add a conversion regression test. Also make the translation canary shell test POSIX-compatible.
Fix the Python ctypes binding for bs_size_convert_to().
BSUnit is a C union containing the binary and decimal unit enums, but
the Python binding previously passed the unit as a plain ctypes.c_int.
Define a matching ctypes.Union and use it for the function argument.
Also fix the conversion unit test to verify that 1 MiB converts to
1024 KiB, and make the translation canary shell test POSIX-compatible.
Tests:
All tests pass.
Summary by CodeRabbit
Bug Fixes
Tests