feat:more encodings #55
Conversation
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces a comprehensive enhancement to the encoding capabilities of the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
hivemind_bus_client/encodings/b100p.py (1)
31-33: Consider additional validation for non-text or complex data.You handle string vs. bytes input well, but if users pass incompatible or unexpected data types, you rely on an
AttributeErroror similar exceptions from.encode(...). Optionally, consider explicitly verifying input type before encoding, to provide a more descriptive error message to the user.setup.py (1)
47-47: Consider usingfind_packages()for scalability.Manually listing subpackages can become error-prone if the project grows. Using
setuptools.find_packages()ensures all submodules are automatically included.hivemind_bus_client/encodings/b91.py (1)
70-72: Docstring return type mismatch with the actual return type.The docstring states it returns a “Base91-encoded string” as
str, but line 100 returnsbytes. Consider aligning the docstring to reflect that the method returns a byte string, or adjust the method to return a Pythonstr.- str: The Base91-encoded string. + bytes: The Base91-encoded data.hivemind_bus_client/encodings/z85b.py (1)
94-100: Improve exception chaining in the decode method.Static analysis recommends raising exceptions with
from Noneorfrom e. This clarifies that the new exception is caused by the caught exception. It can help debug error traces without conflating them with the internal KeyError.except KeyError as e: - raise Z85DecodeError(f"Invalid byte code: {e.args[0]!r}") + raise Z85DecodeError(f"Invalid byte code: {e.args[0]!r}") from None🧰 Tools
🪛 Ruff (0.8.2)
99-99: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
test/test_b100.py (1)
42-46: Correct the docstring to reference B100P instead of Base91.The docstring says “Test decoding with invalid Base91 characters,” but this suite is for B100P. Updating it promotes clarity and consistency.
- """Test decoding with invalid Base91 characters.""" + """Test decoding with invalid B100P characters."""hivemind_bus_client/encryption.py (1)
46-46: Correct the comment to match the new encoding name.
The comment forJSON_Z85Prefers to "Z85B encoding." To avoid confusion, update the comment to referenceZ85Pdirectly.- JSON_Z85P = "JSON-Z85P" # JSON text output with Z85B encoding + JSON_Z85P = "JSON-Z85P" # JSON text output with Z85P encodinghivemind_bus_client/encodings/__init__.py (1)
1-4: Consider referencing or exposing imports to avoid "unused import" warnings.
To address the static analysis hints (F401), either:
- Add these classes to
__all__, or- Use them directly, or
- Alias them if needed for code clarity.
+__all__ = [ + "Z85B", + "Z85P", + "B91", + "B100P", +]🧰 Tools
🪛 Ruff (0.8.2)
1-1:
hivemind_bus_client.encodings.z85b.Z85Bimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
hivemind_bus_client.encodings.z85p.Z85Pimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
hivemind_bus_client.encodings.b91.B91imported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
4-4:
hivemind_bus_client.encodings.b100p.B100Pimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
hivemind_bus_client/encodings/__init__.py(1 hunks)hivemind_bus_client/encodings/b100p.py(1 hunks)hivemind_bus_client/encodings/b91.py(1 hunks)hivemind_bus_client/encodings/z85b.py(1 hunks)hivemind_bus_client/encodings/z85p.py(1 hunks)hivemind_bus_client/encryption.py(2 hunks)hivemind_bus_client/z85b.py(1 hunks)setup.py(1 hunks)test/test_b100.py(1 hunks)test/test_b91.py(1 hunks)test/test_z85b.py(1 hunks)test/test_z85p.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
hivemind_bus_client/z85b.py
3-3: hivemind_bus_client.encodings.z85b.Z85B imported but unused
Remove unused import: hivemind_bus_client.encodings.z85b.Z85B
(F401)
hivemind_bus_client/encodings/z85b.py
99-99: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
hivemind_bus_client/encodings/__init__.py
1-1: hivemind_bus_client.encodings.z85b.Z85B imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: hivemind_bus_client.encodings.z85p.Z85P imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: hivemind_bus_client.encodings.b91.B91 imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
4-4: hivemind_bus_client.encodings.b100p.B100P imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (16)
hivemind_bus_client/encodings/b100p.py (2)
4-12: Good use of docstrings and clear class documentation.The multi-line docstring provides a clear overview of how the class manages encoding, padding bytes, and decoding. This helps maintainers quickly understand its purpose and usage.
38-44: Check for potential integer overflow or invalid byte offsets.The arithmetic
(b + 55) // 64 + 143and(b + 55) % 64 + 128might go out of range ifb + 55significantly exceeds 255 or is negative. Currently, there’s no explicit range check, and while using bytes from standard ASCII or UTF-8 is likely safe, consider adding a small range validation to ensure that invalid input doesn’t produce out-of-bounds emoji codes.hivemind_bus_client/z85b.py (2)
5-11: Well-defined deprecation warning.The warning clearly indicates the new import path. This is helpful for users migrating away from the older module structure.
3-3: Remove or utilize the unused import.Static analysis indicates that
Z85Bis imported but never used. Consider removing it or using it if needed.- from hivemind_bus_client.encodings.z85b import Z85B🧰 Tools
🪛 Ruff (0.8.2)
3-3:
hivemind_bus_client.encodings.z85b.Z85Bimported but unusedRemove unused import:
hivemind_bus_client.encodings.z85b.Z85B(F401)
test/test_b91.py (2)
1-5: Solid test structure for B91 encoding.The test class covers various scenarios (empty, single-byte, edge cases). The straightforward
unittestapproach is easy to maintain.
42-46: Confirm specific error for invalid characters.Raising a
ValueErrorfor invalid Base91 characters is correct. Ensure it’s clear to downstream consumers that they can catchValueErrorto handle malformed input.test/test_z85b.py (2)
1-3: Test suite properly imports the Z85B class and exception.Having explicit imports for both the encoding class and the custom exception clarifies their usage in the tests.
43-46: Validate exception consistency.You correctly test for a
Z85DecodeErroron invalid input. Ensure the encoding class always raises the same exception type on all decode failures, so callers can handle them uniformly.hivemind_bus_client/encodings/b91.py (2)
18-60: Looks great!The decode method’s logic appears correct for Base91. It cleanly handles invalid characters via
ValueError, and the bitwise operations align with standard Base91 decoding patterns. No issues identified.
62-69: Clear and concise encoding logic.The overall approach for assembling bytes, tracking bits, and flushing partial values looks consistent with Base91 encoding methodology. The code is readable and well-structured.
hivemind_bus_client/encodings/z85p.py (1)
1-88: Implementation appears correct and handles padding properly.The use of the first byte to indicate padding length, combined with a
ValueErrorfor invalid sizes, is consistent with standard Z85-based encodings. The big-endian integer packing (">I") and unpacking is appropriate, and the logic for ensuring the encoded length is%5 == 1is correct. No critical issues found.test/test_z85p.py (2)
8-9: Excellent coverage for empty input scenarios.
Testing both byte and string inputs for empty cases is a robust approach. Great job ensuring coverage of edge cases.
95-102: Good verification of padding removal logic.
Ensuring that once thedecodeoperation is performed, any padding is correctly stripped out is a vital check. This test method thoroughly validates that behavior.hivemind_bus_client/encryption.py (3)
49-49: Appreciate the addition of URL-safe Base64.
RenamingJSON_B64to clarify its use of URL-safe Base64 is more descriptive. This helps avoid potential confusion with standard Base64 usage.
66-71: Consistency in get_encoder for new encodings.
The newly introduced encodings (JSON_Z85P,JSON_B91,JSON_B100P) are handled uniformly, aligning with the existing approach for other encodings.
87-92: Symmetrical handling in get_decoder.
The decoder logic mirrors the new encodings added in the encoder, ensuring consistent encoding-decoding behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/unit_tests.yml (1)
3-29: Consider optimizing paths-ignore configuration.The paths-ignore configuration is duplicated between pull_request and push triggers. Consider using a reusable anchor in YAML to maintain these paths in a single location:
name: Run UnitTests on: + # Define paths to ignore + x-ignore-paths: &ignore_paths + - 'hivemind_bus_client/version.py' + - 'examples/**' + - '.github/**' + - '.gitignore' + - 'LICENSE' + - 'CHANGELOG.md' + - 'MANIFEST.in' + - 'readme.md' + - 'scripts/**' pull_request: branches: - dev paths-ignore: - - 'hivemind_bus_client/version.py' - - 'examples/**' - - '.github/**' - - '.gitignore' - - 'LICENSE' - - 'CHANGELOG.md' - - 'MANIFEST.in' - - 'readme.md' - - 'scripts/**' + *: *ignore_paths push: branches: - master paths-ignore: - - 'hivemind_bus_client/version.py' - - 'requirements/**' - - 'examples/**' - - '.github/**' - - '.gitignore' - - 'LICENSE' - - 'CHANGELOG.md' - - 'MANIFEST.in' - - 'readme.md' - - 'scripts/**' + *: *ignore_paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build_tests.yml(1 hunks).github/workflows/unit_tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/unit_tests.yml
40-40: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
60-60: the runner of "codecov/codecov-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (3)
.github/workflows/build_tests.yml (1)
16-16: Verify Python version compatibility requirements.The Python version has been updated from 3.8 to 3.10. This aligns with the Python versions used in the new unit tests workflow (3.9, 3.10, 3.11), but we should ensure:
- All dependencies support Python 3.10
- No breaking changes affect the build process
Run this script to check package compatibility:
✅ Verification successful
Python 3.10 upgrade is compatible with project dependencies
All listed dependencies in
requirements.txtare modern packages that support Python 3.10:
poorman-handshake,ovos_bus_client,ovos_utils: These are actively maintained packagesbitstring>=4.1.1: Supports Python 3.10cryptography>=41.0.1: Modern version with Python 3.10 supportpycryptodomex>=3.18.0: Compatible with Python 3.10pybase64,py-cpuinfo: No version constraints, current versions support Python 3.10No explicit Python version constraints are defined in
setup.py, and all dependencies are using recent versions that are compatible with Python 3.10.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check package dependencies and their Python version support # Get dependencies from setup.py/pyproject.toml rg -A 5 "install_requires|dependencies"Length of output: 354
Script:
#!/bin/bash # Check contents of requirements.txt for dependencies cat requirements.txt # Also check for any Python version constraints in setup.py rg "python_requires" setup.py # Look for any other potential requirements files fd "requirements.*\.txt"Length of output: 252
.github/workflows/unit_tests.yml (2)
34-36: LGTM! Good use of matrix strategy.The matrix strategy with multiple Python versions (3.9, 3.10, 3.11) ensures good coverage across different Python environments.
54-55: Verify test coverage configuration.The pytest command specifies coverage for the
hivemind_bus_clientmodule. Ensure that:
- The coverage includes the new encoding classes
- The test directory path is correct (
test/unittests)Run this script to verify the test structure:
✅ Verification successful
Test coverage configuration is correct and properly structured
The verification shows that:
- All encoding modules have corresponding test files:
b100p.py→test_b100.pyb91.py→test_b91.pyz85b.py→test_z85b.pyz85p.py→test_z85p.py- The test files are correctly located in the
test/directory, which matches the pytest command path- Test files contain proper unit tests for the encoding classes (verified from test_b91.py content)
- The coverage configuration
--cov=hivemind_bus_clientwill include all encoding classes as they are under the specified module🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test directory structure and coverage configuration # Check if test directory exists and list its contents fd -t d "test" # Look for test files related to encodings fd -t f "test_.*\.py" test/Length of output: 121
Script:
#!/bin/bash # Let's verify the test directory structure and content more thoroughly # Check if there's a unittests directory fd -t d "unittests" # Check content of test directory to see if tests are properly organized ls -R test/ # Look for encoding-related files in the source code to ensure test coverage fd -t f ".*\.py" hivemind_bus_client/ | grep -i "encod"Length of output: 353
Script:
#!/bin/bash # Let's verify the test file names match the source files and check test content # Check if test files correspond to source files echo "Source files in hivemind_bus_client/encodings/:" ls hivemind_bus_client/encodings/b*.py hivemind_bus_client/encodings/z*.py echo -e "\nTest files in test/:" ls test/test_*.py # Look at the content of a test file to verify it's testing the encodings head -n 20 test/test_b91.pyLength of output: 1362
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/unit_tests.yml (1)
34-40:⚠️ Potential issueUpdate GitHub Actions to their latest versions
Several actions are using outdated versions that should be updated.
Apply these version updates:
- uses: actions/checkout@v2 + uses: actions/checkout@v4 - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 - uses: codecov/codecov-action@v2 + uses: codecov/codecov-action@v4Also applies to: 55-58
🧰 Tools
🪛 actionlint (1.7.4)
35-35: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
37-37: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (4)
test/requirements.txt (1)
1-1: Consider updating pytest to latest 7.x versionWhile
pytest~=7.1works, consider updating to the latest 7.x version (currently 7.4.3) to benefit from bug fixes and improvements.-pytest~=7.1 +pytest~=7.4.github/workflows/unit_tests.yml (3)
27-33: Consider adding Python 3.12 to the matrixThe matrix strategy is good, but Python 3.12 was released in October 2023 and should be considered for inclusion.
strategy: matrix: - python-version: ["3.10", "3.11" ] + python-version: ["3.10", "3.11", "3.12"]
41-50: Optimize dependency installation stepsConsider these improvements for faster and more reliable builds:
- Use pip's cache feature to speed up installations
- Combine pip install commands to reduce layers
- name: Install System Dependencies run: | sudo apt-get update sudo apt install python3-dev swig - python -m pip install build wheel - - name: Install repo - run: | - pip install -e . - - name: Install test dependencies - run: | - pip install -r test/requirements.txt + - name: Install Python Dependencies + run: | + python -m pip install --upgrade pip build wheel + pip install -e . -r test/requirements.txtAlso, add pip caching:
- uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} cache: 'pip'
51-54: Consider adding test result artifactsThe test step could be enhanced by uploading test results as artifacts for better debugging.
- name: Run unittests run: | - pytest --cov=hivemind_bus_client --cov-report xml test + pytest --cov=hivemind_bus_client --cov-report xml --junitxml=junit/test-results.xml test + - name: Upload Test Results + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-results-${{ matrix.python-version }} + path: junit/test-results.xml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/unit_tests.yml(1 hunks)test/requirements.txt(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/unit_tests.yml
35-35: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
37-37: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
58-58: the runner of "codecov/codecov-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
test/requirements.txt (1)
2-2: LGTM! Good practice adding coverage reportingAdding pytest-cov is excellent for ensuring test coverage of the new encoding implementations.
.github/workflows/unit_tests.yml (1)
1-26: LGTM: Well-structured workflow triggersThe workflow triggers and path exclusions are well defined, covering both PR to dev and push to master branches while appropriately ignoring documentation and configuration files.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
even more encodings, following up #50
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
setup.pyto include newencodingssubpackage.pytestandpytest-covas dependencies for testing.