Skip to content

Conversation

@sitaowang1998
Copy link
Collaborator

@sitaowang1998 sitaowang1998 commented Nov 1, 2025

Description

This PR:

  • Adds parsing of native bytes type.
  • Adds conversion from TDL bytes to native bytes.
  • Adds unit tests for parsing and conversion of bytes.
  • Adds unit tests for serializing/deserializing bytes objects.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Newly added unit tests pass.
  • GitHub workflows pass.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced bytes as a supported primitive type throughout the type system.
    • Extended type definitions and conversions to handle bytes and List collections.
  • Tests

    • Added comprehensive test coverage for bytes serialization, deserialization, type parsing, and collection handling.

@sitaowang1998 sitaowang1998 requested a review from a team as a code owner November 1, 2025 04:01
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 1, 2025

Walkthrough

This pull request adds support for Python's bytes type as a primitive TDL (Type Definition Language) type. A new BytesType class is introduced and integrated into type conversion, parsing, and serialization systems with corresponding test coverage for bytes in primitive and collection contexts.

Changes

Cohort / File(s) Summary
Type definition
python/spider-py/src/spider_py/type/tdl_type.py
Introduces new BytesType class implementing TdlType interface with type_str() returning "bytes" and native_type() returning bytes.
Type mappings
python/spider-py/src/spider_py/type/tdl_convert.py, python/spider-py/src/spider_py/type/tdl_parse.py
Adds BytesType import and includes bytes: BytesType() entry in TypeDict (convert) and PrimitiveTypeDict (parse) to enable bytes type resolution. Removes bytes from error-raising path in conversion.
Test coverage
python/spider-py/tests/type/test_to_native.py, python/spider-py/tests/type/test_to_tdl.py
Extends type conversion tests to verify bytes as primitive type maps correctly and supports List<bytes> context. Removes prior TypeError expectation for bytes.
Serialization tests
python/spider-py/tests/utils/test_serde.py
Adds serialization and deserialization test cases for bytes type and list[bytes].

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Type definition (tdl_type.py): Straightforward new class following existing TdlType pattern
  • Type mappings (convert/parse): Direct dictionary entries with consistent structure
  • Test additions: Repetitive test case patterns, covering bytes in isolation and within List contexts
  • All changes follow established conventions with minimal logic density

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "feat(spider-py): Adds bytes type support in TDL." directly and accurately reflects the primary objective of the changeset. The entire PR focuses on implementing bytes type support across three core modules (tdl_type.py introduces BytesType, tdl_convert.py and tdl_parse.py add bytes mappings), along with corresponding test coverage across four test files. The title is concise, specific, and clearly communicates the main change without vague language or unnecessary details. A developer reviewing the PR history would immediately understand the scope and intent.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 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 8e90883 and d79c6f6.

📒 Files selected for processing (6)
  • python/spider-py/src/spider_py/type/tdl_convert.py (3 hunks)
  • python/spider-py/src/spider_py/type/tdl_parse.py (2 hunks)
  • python/spider-py/src/spider_py/type/tdl_type.py (1 hunks)
  • python/spider-py/tests/type/test_to_native.py (2 hunks)
  • python/spider-py/tests/type/test_to_tdl.py (1 hunks)
  • python/spider-py/tests/utils/test_serde.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
python/spider-py/src/spider_py/type/tdl_parse.py (1)
python/spider-py/src/spider_py/type/tdl_type.py (1)
  • BytesType (113-122)
python/spider-py/tests/type/test_to_tdl.py (1)
python/spider-py/src/spider_py/type/tdl_convert.py (1)
  • to_tdl_type_str (97-103)
python/spider-py/src/spider_py/type/tdl_convert.py (1)
python/spider-py/src/spider_py/type/tdl_type.py (13)
  • BytesType (113-122)
  • native_type (25-26)
  • native_type (37-38)
  • native_type (49-50)
  • native_type (61-62)
  • native_type (73-74)
  • native_type (85-86)
  • native_type (97-98)
  • native_type (109-110)
  • native_type (121-122)
  • native_type (140-145)
  • native_type (163-164)
  • native_type (212-213)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (11)
python/spider-py/tests/utils/test_serde.py (1)

55-56: LGTM! Well-integrated test coverage for bytes.

The test cases for bytes and list[bytes] follow the established pattern and provide appropriate coverage for the new primitive type.

python/spider-py/tests/type/test_to_native.py (2)

38-38: LGTM! Correct primitive type mapping.

The assertion correctly verifies that the TDL string "bytes" maps to the native bytes type.


52-52: LGTM! Correct list type mapping.

The assertion correctly verifies that "List" maps to list[bytes], consistent with other list type tests.

python/spider-py/src/spider_py/type/tdl_parse.py (2)

7-7: LGTM! Correct import added.

BytesType is properly imported to support bytes as a primitive TDL type.


35-35: LGTM! Bytes primitive registered correctly.

The PrimitiveTypeDict entry enables parsing of "bytes" as a TDL primitive type, consistent with other primitive registrations.

python/spider-py/tests/type/test_to_tdl.py (2)

21-21: LGTM! Bytes primitive conversion tested.

The assertion correctly verifies that native bytes type converts to TDL string "bytes".


27-27: LGTM! List of bytes conversion tested.

The assertion correctly verifies that list[bytes] converts to "List", consistent with other list type tests.

python/spider-py/src/spider_py/type/tdl_type.py (1)

113-123: LGTM! BytesType implementation is clean and consistent.

The BytesType class follows the established pattern for primitive TDL types, with proper overrides and correct return values for type_str() and native_type().

python/spider-py/src/spider_py/type/tdl_convert.py (3)

11-11: LGTM! Import added correctly.

BytesType is properly imported to enable bytes type conversion.


34-34: LGTM! Bytes type registered in TypeDict.

The TypeDict entry correctly maps native bytes to BytesType(), enabling type resolution in _to_primitive_tdl_type.


50-50: LGTM! Error path correctly updated.

Removing bytes from the explicit error tuple is correct, as bytes is now handled through the TypeDict mapping above.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant