Replace non-unicode characters for add-on static files #5712
Conversation
agners
added
the
refactor
📝 WalkthroughWalkthroughThe changes involve the refactoring of file reading operations within the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as APIStore
participant T as Text Reader
participant B as Binary Reader
C->>A: Request addon changelog/documentation
A->>T: Call _read_static_text_file(path)
T-->>A: Return text content
A-->>C: Respond with text asset
C->>A: Request addon icon/logo
A->>B: Call _read_static_binary_file(path)
B-->>A: Return binary content
A-->>C: Respond with binary asset
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (8)
✨ Finishing Touches
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/api/store.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
- GitHub Check: Check ruff
🔇 Additional comments (5)
supervisor/api/store.py (5)
72-78: Good implementation of text file handling with error replacement.This function correctly implements reading text files with unicode error handling by using the
errors="replace"parameter. This will replace non-unicode characters with the unicode replacement character (�), allowing files with encoding issues to be properly displayed instead of throwing UnicodeDecodeError exceptions.
81-87: Binary file reader function properly implemented.The separation of binary file reading into its own function maintains clean code separation. Binary files don't need error replacement since they're not interpreted as text.
281-281: Text file reading with error replacement correctly implemented.This change properly implements the new
_read_static_text_filefunction for changelog files, which will handle non-unicode characters gracefully by replacing them with the unicode replacement character.
295-297: Text file reading with error replacement correctly implemented for documentation.The implementation correctly uses the new
_read_static_text_filefunction for documentation files, which will handle non-unicode characters properly.
72-297:✅ Verification successful
Verify all usages of the replaced function in the codebase.
This PR replaces a generic
_read_static_filefunction with two specialized functions. Let's verify that all usages of the previous function have been updated throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of the old _read_static_file function echo "Checking for remaining usages of _read_static_file:" rg "_read_static_file" --type py # Check if there are any other places where files are read that might benefit from the same error handling approach echo "Looking for other file opening patterns that might need similar error handling:" rg "\.open\(" --type py | grep -v "errors=\"replace\""Length of output: 2363
All usages of the old
_read_static_filehave been successfully removed
- We verified that no instances of
_read_static_fileremain in the codebase.- The new specialized functions (
_read_static_text_filewitherrors="replace"for text files and_read_static_binary_filefor binary files) are correctly used insupervisor/api/store.py.- The other file opening patterns (e.g., in tests) do not interfere with this change and use the appropriate modes for their specific purposes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
supervisor/api/store.py (2)
81-87: Consider adding a specific return type annotation.The function signature uses
Anyas the return type, but it's always returning bytes for binary files. Consider improving type safety by using a more specific return type.-def _read_static_binary_file(path: Path) -> Any: +def _read_static_binary_file(path: Path) -> bytes:
72-72: Consider adding a specific return type annotation.Similar to the binary file function, the text file function would benefit from a more specific return type instead of
Any.-def _read_static_text_file(path: Path) -> Any: +def _read_static_text_file(path: Path) -> str:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/api/store.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (5)
supervisor/api/store.py (5)
72-78: Good handling of Unicode decoding errors in text files.The implementation of
_read_static_text_filewitherrors="replace"parameter addresses the issue with non-unicode characters or file corruption. This will replace problematic characters with the Unicode replacement character "�" rather than causing the entire file read to fail.
259-259: LGTM! The function now correctly reads binary data for icons.This change properly separates the binary file reading logic, which is appropriate for image files like icons.
268-268: LGTM! The function now correctly reads binary data for logos.This change properly separates the binary file reading logic, which is appropriate for image files like logos.
282-284: Good enhancement to handle Unicode errors in changelog files.Using
_read_static_text_filewith error replacement will prevent UnicodeDecodeError and improve user experience by displaying content even with problematic characters.
298-300: Good enhancement to handle Unicode errors in documentation files.Using
_read_static_text_filewith error replacement will prevent UnicodeDecodeError and improve user experience by displaying content even with problematic characters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/api/test_store.py (1)
378-378: Fix the docstring to match the test purpose.The docstring incorrectly states that this is testing the documentation API endpoint, but it's actually testing the changelog endpoint.
- """Test /store/addons/{addon}/documentation REST API. + """Test /store/addons/{addon}/changelog REST API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/api/test_store.py(1 hunks)tests/conftest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
tests/conftest.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
tests/conftest.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
tests/conftest.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
tests/conftest.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
tests/conftest.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
tests/conftest.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (3)
tests/conftest.py (1)
519-519: Ensures test paths are available to addons in store.This addition properly sets up the addon's location path in the store data, which is necessary for the new tests handling corrupted text files to access physical file paths.
tests/api/test_store.py (2)
354-370: Good test for handling corrupted documentation files.This test properly verifies that the API can handle documentation files with invalid UTF-8 characters, replacing them with the Unicode replacement character (�) instead of failing with a UnicodeDecodeError. This aligns with the PR objective of improving user experience when encountering non-unicode characters.
373-389: Good test for handling corrupted changelog files.This test correctly verifies that the API can handle changelog files with invalid UTF-8 characters, ensuring they're properly replaced with the Unicode replacement character (�) rather than failing with an exception.
agners
force-pushed
the
replace-non-unicode-characters-in-text-files
branch
from
85756a6 to
096e48c
Compare
Add-on documentation and changelog get read and returned as text file. However, in case the original author used non-unicode characters, or the file corrupted, loading currently fails with an UnicodeDecodeError. Let's just use the built-in replace error handling of Python, so they appear for the user as non-unicode characters by replacing them with the official unicode replacement character "�".
agners
force-pushed
the
replace-non-unicode-characters-in-text-files
branch
from
096e48c to
4495b85
Compare
Proposed change
Add-on documentation and changelog get read and returned as text file. However, in case the original author used non-unicode characters, or the file corrupted, loading currently fails with an UnicodeDecodeError.
Let's just use the built-in replace error handling of Python, so they appear for the user as non-unicode characters by replacing them with the official unicode replacement character "�".
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit