Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ refactor encoding utilities #12

Merged
merged 5 commits into from
Feb 9, 2025
Merged

♻️ refactor encoding utilities #12

merged 5 commits into from
Feb 9, 2025

Conversation

techouse
Copy link
Owner

@techouse techouse commented Feb 9, 2025

This pull request includes significant changes to the encode_utils.py file to improve the encoding utilities and refactor the code for better readability and maintainability. Additionally, it updates the unit tests to reflect these changes.

Improvements to encoding utilities:

  • src/qs_codec/utils/encode_utils.py: Refactored the escape method to use predefined sets of safe characters, improving readability and maintainability.
  • src/qs_codec/utils/encode_utils.py: Introduced new methods _convert_value_to_string, _encode_string, _is_safe_char, and _encode_char to modularize the encoding logic, making the code easier to understand and extend.
  • src/qs_codec/utils/encode_utils.py: Modified the HEX_TABLE initialization for simplicity and added several sets of safe characters for different encoding formats.

Refactoring and renaming:

Updates to unit tests:

  • tests/unit/utils_test.py: Updated tests to use type annotations and reflect changes in method names and signatures. Added new tests for the _is_safe_char method to ensure correctness. [1] [2] [3]

Summary by CodeRabbit

  • Refactor

    • Enhanced the encoding process to improve string conversion and overall reliability, resulting in more efficient character handling.
  • Tests

    • Expanded test coverage with additional validations and type checks to ensure robust character processing and maintain high quality.

@techouse techouse added the enhancement New feature or request label Feb 9, 2025
@techouse techouse self-assigned this Feb 9, 2025
Copy link

coderabbitai bot commented Feb 9, 2025

Walkthrough

The changes update the encoding logic in the EncodeUtils class by simplifying the initialization of the HEX_TABLE and introducing new class-level constants for safe character checks. The escape method has been refactored for improved readability and efficiency, utilizing direct character iteration and a unified set of safe characters. Additionally, the encode method has been modularized into private methods for better structure. The test suite has also been updated to reflect these changes, including type annotations and a new test for character safety.

Changes

File Path Change Summary
src/qs_codec/utils/encode_utils.py Simplified HEX_TABLE initialization; refactored escape method for direct character iteration and unified safe character checks; added new constants for safe characters; renamed to_surrogates to _to_surrogates and updated internal variable name from result to buffer.
tests/unit/utils_test.py Added type annotations for variables in test methods; renamed method from to_surrogates to _to_surrogates; introduced new test method test_is_safe_char for character safety checks.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant EncodeUtils
    Client->>EncodeUtils: escape(input_string)
    EncodeUtils->>EncodeUtils: For each character in input_string
    alt Character is safe
        EncodeUtils->>EncodeUtils: Append character to output buffer
    else Character is unsafe
        EncodeUtils->>EncodeUtils: Lookup HEX_TABLE and append formatted hex code
    end
    EncodeUtils-->>Client: Return escaped string
Loading

Poem

I'm a rabbit on the run,
Hopping through the lines of code,
HEX tables now shine like a sun,
Escape loops lighten the load.
With ASCII dreams and hops so free,
I celebrate these changes with glee! 🐇
CodeRabbit cheers for clarity!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8709c8e and 121531b.

📒 Files selected for processing (2)
  • src/qs_codec/utils/encode_utils.py (4 hunks)
  • tests/unit/utils_test.py (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (10)
src/qs_codec/utils/encode_utils.py (7)

17-17: LGTM! Clean and efficient HEX_TABLE initialization.

The list comprehension provides a concise and readable way to generate the hex table.


20-33: LGTM! Well-organized safe character sets.

The safe character sets are clearly defined, well-documented, and follow RFC standards. The type annotations enhance code clarity.


35-61: LGTM! Improved escape method implementation.

The refactored escape method is more efficient and readable, utilizing the new safe character sets. The redundant RFC1738 safe points addition has been correctly removed.


63-115: LGTM! Well-structured encoding logic.

The encoding logic has been effectively modularized into smaller, focused methods, improving code organization and maintainability.


117-121: LGTM! Clear and focused character safety check.

The _is_safe_char method provides a clean interface for checking character safety using the new safe character sets.


122-154: LGTM! Comprehensive character encoding implementation.

The character encoding methods handle all UTF-8 cases correctly, with clear documentation and proper type annotations.


156-173: LGTM! Properly renamed and refactored surrogate conversion.

The method has been correctly renamed to indicate its private nature, maintaining the same correct implementation.

tests/unit/utils_test.py (3)

486-522: LGTM! Enhanced type safety with annotations.

The added type annotations improve code clarity and type safety.


581-582: LGTM! Correctly updated test for renamed method.

The test has been properly updated to use the renamed _to_surrogates method.


584-610: LGTM! Comprehensive test coverage for character safety.

The parametrized tests thoroughly cover character safety checks for both RFC3986 and RFC1738 formats, including edge cases.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.01% (target: -1.00%) 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (abdf924) 739 714 96.62%
Head commit (b7ea34b) 741 (+2) 716 (+2) 96.63% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#12) 11 11 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codacy-production bot commented Feb 9, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.09% (target: -1.00%) 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (abdf924) 739 714 96.62%
Head commit (121531b) 760 (+21) 735 (+21) 96.71% (+0.09%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#12) 58 58 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codecov bot commented Feb 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.71%. Comparing base (abdf924) to head (121531b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   96.61%   96.71%   +0.09%     
==========================================
  Files          16       16              
  Lines         739      760      +21     
==========================================
+ Hits          714      735      +21     
  Misses         25       25              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@techouse techouse marked this pull request as ready for review February 9, 2025 16:22
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/qs_codec/utils/encode_utils.py (1)

30-37: Consider making the safe set a class constant.

The safe set is currently recreated on each method call. Moving it to a class constant would improve performance by avoiding repeated set creation.

 class EncodeUtils:
     HEX_TABLE: t.Tuple[str, ...] = tuple(f"%{i:02X}" for i in range(256))
+    # Safe code points for URL encoding
+    SAFE_POINTS: t.Set[int] = (
+        set(range(0x30, 0x3A))  # 0-9
+        | set(range(0x41, 0x5B))  # A-Z
+        | set(range(0x61, 0x7B))  # a-z
+        | {0x40, 0x2A, 0x5F, 0x2D, 0x2B, 0x2E, 0x2F}  # @, *, _, -, +, ., /
+    )
+    # Additional safe points for RFC1738
+    RFC1738_SAFE_POINTS: t.Set[int] = SAFE_POINTS | {0x28, 0x29}  # ( )

     @classmethod
     def escape(
         cls,
         string: str,
         format: t.Optional[Format] = Format.RFC3986,
     ) -> str:
-        # Build a set of "safe" code points.
-        safe: t.Set[int] = set(range(0x30, 0x3A)) | set(range(0x41, 0x5B)) | set(range(0x61, 0x7B))
-        safe |= {0x40, 0x2A, 0x5F, 0x2D, 0x2B, 0x2E, 0x2F}  # @, *, _, -, +, ., /
-
-        # For RFC1738, add the ASCII codes for ( and )
-        if format == Format.RFC1738:
-            safe |= {0x28, 0x29}
+        safe = cls.RFC1738_SAFE_POINTS if format == Format.RFC1738 else cls.SAFE_POINTS
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between abdf924 and b7ea34b.

📒 Files selected for processing (1)
  • src/qs_codec/utils/encode_utils.py (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/qs_codec/utils/encode_utils.py (3)

17-17: Great optimization of HEX_TABLE initialization!

The new implementation using f-string formatting is more concise and efficient while maintaining the same functionality.


30-37: Well-structured safe character definition!

The use of set operations and explicit ranges makes the code more readable and maintainable.


40-51: Clean and efficient character processing!

The changes improve both readability and performance:

  • Direct character iteration is more Pythonic
  • Set membership testing is efficient
  • Clear separation of encoding paths

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7ea34b and 8709c8e.

📒 Files selected for processing (1)
  • src/qs_codec/utils/encode_utils.py (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/qs_codec/utils/encode_utils.py (3)

17-17: LGTM! Clean and efficient HEX_TABLE initialization.

The simplified initialization using f-strings improves readability while maintaining functionality.


40-60: LGTM! Well-structured escape method with efficient set-based lookups.

The refactored code is more maintainable and performs better by:

  • Using precomputed sets for efficient character validation
  • Employing consistent string formatting
  • Clearly separating the logic for different character types

149-163: LGTM! Consistent variable naming.

The changes maintain consistency with other methods by using 'buffer' instead of 'result'.

src/qs_codec/utils/encode_utils.py Outdated Show resolved Hide resolved
@techouse techouse marked this pull request as draft February 9, 2025 17:00
@techouse techouse marked this pull request as ready for review February 9, 2025 17:27
@techouse techouse changed the title ♻️ refactor ♻️ refactor encoding utilities Feb 9, 2025
@techouse techouse merged commit 2d94487 into main Feb 9, 2025
15 of 16 checks passed
@techouse techouse deleted the chore/refactor branch February 9, 2025 17:51
@coderabbitai coderabbitai bot mentioned this pull request Feb 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant