Conversation
WalkthroughAdds Pydantic-based ParsleyObject/ParsleyError models, a static _ParsleyParseInternal parser with helper functions, four concrete ParsleyParser implementations, modernized type hints and deprecations on legacy parse APIs, and declares runtime dependency Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Parser as Parser\n(USBDebug/Logger/Live/Bitstring)
participant Internal as _ParsleyParseInternal
participant Validator as Pydantic\nParsleyObject
participant Result as ParsleyObject/\nParsleyError
Client->>Parser: parse(input)
activate Parser
Parser->>Internal: parse_to_object(msg_sid, msg_data)
activate Internal
Internal->>Internal: decode SID & board IDs\nlookup message definition\nparse bit fields
Internal->>Validator: construct ParsleyObject(data)
activate Validator
Validator->>Validator: validate msg_prio\nvalidate msg_type
Validator-->>Internal: validated object
deactivate Validator
alt success
Internal-->>Parser: ParsleyObject
else failure
Internal-->>Parser: ParsleyError
end
deactivate Internal
Parser-->>Client: ParsleyObject / ParsleyError
deactivate Parser
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parsley/parsley.py (1)
32-72: Fix return type annotation for parse().The function signature indicates it returns
ParsleyObject, but line 61 returnsParsleyError. The return type should beUnion[ParsleyObject, ParsleyError].Apply this diff:
-def parse(msg_sid: bytes, msg_data: bytes) -> ParsleyObject: +def parse(msg_sid: bytes, msg_data: bytes) -> Union[ParsleyObject, ParsleyError]:Additionally, line 50 has an invalid type annotation
data: T = {}. This should be:- data: T = {} + data: dict = {}
🧹 Nitpick comments (3)
src/parsley/__init__.py (1)
15-15: Consider exporting ParsleyError as well.Since
parse()can returnParsleyError, users of the library may need to check for or handle error cases. ExportingParsleyErrorwould make the public API more complete and allow users to writeisinstance(result, parsley.ParsleyError)checks.Apply this diff:
-from .parsley_message import ParsleyObject +from .parsley_message import ParsleyObject, ParsleyErrorsrc/parsley/parsley_message.py (2)
12-17: Consider whether ParsleyError should inherit from Exception.
ParsleyErroris a dataclass but doesn't inherit fromException. This means it cannot be raised or caught in try-except blocks. Based on the usage insrc/parsley/parsley.py(lines 61-64), it's being returned rather than raised, which is acceptable. However, the name "Error" typically implies an exception class.Consider either:
- Renaming to
ParsleyParseFailureorParsleyParseResultto clarify it's a return value, not an exception- Inheriting from
Exceptionif you plan to raise it in the future
31-59: Simplify the equality implementation.The current implementation is verbose and repetitive. It can be significantly simplified using early returns and standard comparison.
Apply this diff:
def __eq__(self, other: object) -> bool: #allows comparison to dicts if isinstance(other, dict): - isSame = True - - if self.board_type_id != other.get('board_type_id'): - isSame = False - if self.board_inst_id != other.get('board_inst_id'): - isSame = False - if self.msg_prio != other.get('msg_prio'): - isSame = False - if self.msg_type != other.get('msg_type'): - isSame = False - if self.data != other.get('data'): - isSame = False - - return isSame - if isinstance(other, ParsleyObject): - isSame = True - if self.board_type_id != other.board_type_id: - isSame = False - if self.board_inst_id != other.board_inst_id: - isSame = False - if self.msg_prio != other.msg_prio: - isSame = False - if self.msg_type != other.msg_type: - isSame = False - if self.data != other.data: - isSame = False - return isSame + return ( + self.board_type_id == other.get('board_type_id') and + self.board_inst_id == other.get('board_inst_id') and + self.msg_prio == other.get('msg_prio') and + self.msg_type == other.get('msg_type') and + self.data == other.get('data') + ) + if isinstance(other, ParsleyObject): + return ( + self.board_type_id == other.board_type_id and + self.board_inst_id == other.board_inst_id and + self.msg_prio == other.msg_prio and + self.msg_type == other.msg_type and + self.data == other.data + ) + return NotImplemented
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pyproject.toml(2 hunks)src/parsley/__init__.py(1 hunks)src/parsley/message_types.py(0 hunks)src/parsley/parsley.py(4 hunks)src/parsley/parsley_message.py(1 hunks)tests/test_parsley.py(3 hunks)
💤 Files with no reviewable changes (1)
- src/parsley/message_types.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/parsley/__init__.py (1)
src/parsley/parsley_message.py (1)
ParsleyObject(20-70)
tests/test_parsley.py (3)
src/parsley/parsley_message.py (1)
ParsleyError(13-17)src/parsley/parsley.py (1)
parse(32-72)tests/utils.py (1)
create_msg_sid_from_strings(11-24)
src/parsley/parsley.py (4)
src/parsley/bitstring.py (1)
BitString(1-49)src/parsley/fields.py (10)
Field(6-36)Switch(181-197)Bitfield(199-232)decode(18-23)decode(49-50)decode(91-96)decode(122-124)decode(162-166)decode(213-228)get_fields(193-194)src/parsley/parsley_message.py (2)
ParsleyObject(20-70)ParsleyError(13-17)src/parsley/parse_utils.py (1)
hexify(1-13)
🪛 Ruff (0.14.3)
src/parsley/parsley.py
87-87: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
tests/test_parsley.py (5)
112-112: LGTM!The updated error handling correctly checks for
ParsleyErrorinstances instead of inspecting dict error keys.
118-118: LGTM!Correctly verifies error case with
isinstancecheck.
124-124: LGTM!Properly checks for
ParsleyErroron invalid SID input.
147-147: LGTM!Correctly updated to check for
ParsleyErrorwhen message data is malformed.
137-139: Test expectations verified as correct and properly aligned with parse behavior.The verification confirms that
parse()successfully handles invalid board type IDs by returning a fallback hex value. When an invalid board type ID (0x1F) is encountered,parse_board_type_id()catches the ValueError during enum decoding and applies thehexify()fallback, which returns a hex string with the '0x' prefix. Theparse()function returns a validParsleyObjectwithout raising an error, and the test assertion checking for '0x' inboard_type_idis correct.src/parsley/parsley_message.py (1)
61-70: Handle invalid keys in getitem.The current implementation will raise a
KeyErrorif an invalid key is accessed, which is the correct dict behavior. The implementation looks good and properly supports dict-like access for backwards compatibility.src/parsley/parsley.py (1)
157-157: LGTM!The return type
Iterator[Tuple[bytes, bytes]]correctly reflects that this is a generator function (usesyieldon line 203).pyproject.toml (1)
21-21: No issues found.The latest pydantic version on PyPI is 2.12.3, matching the PR requirement. Known security advisories for pydantic 2.x only affect versions < 2.4.0 (regex DoS from 2024); version 2.12.3 is well above this threshold and has no applicable vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parsley/parsley.py (1)
32-72: Fix parse()'s inconsistent return typeOn parse failures this function now returns a
ParsleyError, yet the signature still promisesParsleyObject. Callers (seeparse_message) will keep assuming the happy path and crash. Either makeparse()consistently yield aParsleyObjecteven for errors, or update the signature toUnion[ParsleyObject, ParsleyError]and propagate that expectation.-def parse(msg_sid: bytes, msg_data: bytes) -> ParsleyObject: +def parse(msg_sid: bytes, msg_data: bytes) -> Union[ParsleyObject, ParsleyError]:
♻️ Duplicate comments (2)
src/parsley/parsley.py (2)
2-28: Remove the unused TypeVar T
parse_fieldsreturns a heterogenousdictof field values, so the newTypeVarnever captures a single type and just reintroduces the incorrect hint from the last review. Please dropTypeVarand annotate withdict[str, Any]instead.-from typing import List, Tuple, Union, Iterator, TypeVar +from typing import Any, Iterator, List, Tuple, Union @@ -T = TypeVar("T") - -def parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, T]: +def parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, Any]:
84-96: Tighten exception handling in to_enum()The broad
except Exceptionfrom the earlier revision is still here, and the extra in-function import remains redundant. Please narrow the exception to the expectedKeyError/AttributeErrorand rely on the module-levelmtimport.def to_enum(enum_cls, name: str): try: return enum_cls[name] - except Exception: + except (KeyError, AttributeError): return name - - import parsley.message_types as mt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/parsley/parsley.py(4 hunks)src/parsley/parsley_message.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/parsley/parsley_message.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/parsley/parsley.py (4)
src/parsley/bitstring.py (1)
BitString(1-49)src/parsley/fields.py (10)
Field(6-36)Switch(181-197)Bitfield(199-232)decode(18-23)decode(49-50)decode(91-96)decode(122-124)decode(162-166)decode(213-228)get_fields(193-194)src/parsley/parsley_message.py (2)
ParsleyObject(19-72)ParsleyError(13-17)src/parsley/parse_utils.py (1)
hexify(1-13)
🪛 Ruff (0.14.3)
src/parsley/parsley.py
87-87: Do not catch blind exception: Exception
(BLE001)
ChrisYx511
left a comment
ChrisYx511
left a comment
There was a problem hiding this comment.
In general, let's try not making this a fully breaking change and also take the opportunity to cleanup the parser code while we're at it.
|
pls rebase with latest main branch |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
src/parsley/__init__.py (1)
5-5: Inconsistency:ParsleyObjectandParsleyErrorare imported but excluded from__all__.These classes are imported at line 5 but not listed in
__all__. If they should be part of the public API, add them to__all__. If not, consider whether this import is needed at package level.Apply this diff to include them in the public API:
__all__ = ["parse_fields", "parse", "parse_board_type_id", "parse_board_inst_id", "parse_bitstring", "parse_live_telemetry", "parse_usb_debug", "parse_logger", "format_line", "encode_data", - "calculate_msg_bit_len"] + "calculate_msg_bit_len", + "ParsleyObject", + "ParsleyError"]Also applies to: 17-24
src/parsley/parsley_message.py (2)
2-2: Remove unusedLiteralimport.
Literalis imported but never used in this file.-from typing import Literal, Generic, TypeVar +from typing import Generic, TypeVar
13-14:Generic[T]onParsleyErroris unused.The type parameter
Tis declared but never referenced inParsleyError's fields. Either remove the generic or use it (e.g., formsg_datatyping).@dataclass -class ParsleyError(Generic[T]): +class ParsleyError: """Custom error container for Parsley errors."""src/parsley/parsley.py (3)
4-4: Remove unusedBaseModelimport.
BaseModelis imported but not used in this file.-from pydantic import BaseModel
13-15: IncorrectTypeVarusage in return type.
T = TypeVar("T")is unbound here, sodict[str, T]doesn't provide meaningful type information. UseAnyinstead.-T = TypeVar("T") - -def parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, T]: +from typing import Any + +def parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, Any]:
119-119: Return type annotation includesNonebut function is a generator.
parse_loggeris a generator (usesyield), so its return type should beIterator[Tuple[bytes, bytes]]rather thanUnion[Tuple[bytes, bytes], None].-def parse_logger(buf: bytes, page_number: int) -> Union[Tuple[bytes, bytes], None]: +def parse_logger(buf: bytes, page_number: int) -> Iterator[Tuple[bytes, bytes]]:tests/test_parse_to_object.py (2)
181-183: Remove unused exception variablee.The
as ecaptures are not used. Per Ruff F841.- with pytest.raises(IndexError) as e: + with pytest.raises(IndexError): BitstringParser().parse(bit_str)Also applies to: 188-190
203-203: Use equality check instead of tuple membership.
res['data']['msg_data'] in ('0x0')checks if the value is in a single-element tuple. Use==for clarity.- assert res['data']['msg_data'] in ('0x0') + assert res['data']['msg_data'] == '0x0'src/parsley/parse_to_object.py (3)
15-15: IncorrectTypeVarusage - should useAny.
T = TypeVar("T")is unbound and used indict[str, T], which doesn't provide meaningful type information.-T = TypeVar("T") +from typing import AnyAnd update the function signature:
- def _parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, T]: + def _parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, Any]: ... - res: dict[str, T] = {} + res: dict[str, Any] = {}Also applies to: 68-75
87-92: Return type includesNonebut function never returnsNone.
_format_can_messagealways returns a tuple, so removeNonefrom the union.@staticmethod - def _format_can_message(msg_sid: int, msg_data: List[int]) -> Union[Tuple[bytes, bytes], None]: + def _format_can_message(msg_sid: int, msg_data: List[int]) -> Tuple[bytes, bytes]:
217-220: Replace en-dash characters with hyphens.The docstring uses en-dash (–) characters which can cause issues. Use standard hyphens (-) instead.
- 0 – 2 : ASCII 'L','O','G' - 4 – 12 : SID (uint32 LE) | timestamp (uint32 LE) | DLC (uint8) - 13 – .. : up to 8 bytes CAN payload + 0 - 2 : ASCII 'L','O','G' + 4 - 12 : SID (uint32 LE) | timestamp (uint32 LE) | DLC (uint8) + 13 - .. : up to 8 bytes CAN payload
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/parsley/__init__.py(2 hunks)src/parsley/parse_to_object.py(1 hunks)src/parsley/parsley.py(2 hunks)src/parsley/parsley_message.py(1 hunks)tests/test_parse_to_object.py(1 hunks)tests/test_parsley.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/test_parsley.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/parsley/parse_to_object.py (5)
src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)src/parsley/bitstring.py (3)
BitString(1-49)push(30-41)pop(12-28)src/parsley/fields.py (4)
Field(6-36)Switch(181-197)Bitfield(199-232)get_fields(193-194)src/parsley/parsley.py (4)
format_line(201-211)calculate_msg_bit_len(214-218)encode_data(175-193)parse(34-65)src/parsley/parse_utils.py (1)
hexify(1-13)
src/parsley/parsley.py (1)
src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)
src/parsley/__init__.py (1)
src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)
🪛 Ruff (0.14.5)
src/parsley/parse_to_object.py
177-177: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
197-197: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Avoid specifying long messages outside the exception class
(TRY003)
217-217: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
219-219: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
220-220: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
238-238: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Avoid specifying long messages outside the exception class
(TRY003)
252-252: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_parse_to_object.py
181-181: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
188-188: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
src/parsley/__init__.py
17-24: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
src/parsley/parsley_message.py
39-39: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
src/parsley/parsley_message.py (1)
36-46: Validators look good.The runtime validation for
msg_prioandmsg_typeagainstmt.msg_prioandmt.msg_typedictionaries provides useful runtime checks. This aligns with the Pydantic approach.tests/test_parse_to_object.py (2)
25-52: Good test coverage for successful parsing.The test thoroughly validates all fields of the parsed result including board IDs, message priority, type, and nested data fields with proper approximation for floating-point comparison.
284-328: Comprehensive logger parsing tests.Good coverage of the logger parsing including valid multi-message scenarios, wrong size, wrong signature, wrong page number, and empty buffer cases.
src/parsley/parse_to_object.py (3)
23-26: Good static class pattern.Raising
NotImplementedErrorin__init__effectively prevents instantiation, enforcing static-only usage.
164-169: Abstract base class is well-structured.The
ParsleyParserABC provides a clean interface for different input format parsers.
112-162: Core parsing logic handles errors gracefully.The
parse_to_objectmethod properly handles both int and bytes SID, catches parsing errors, and returns appropriateParsleyErrorobjects with context.
ChrisYx511
left a comment
ChrisYx511
left a comment
There was a problem hiding this comment.
implementation looks good! please add unit tests! you can probably just use existing tests with some minor modifications.
i haven't looked too too in detail though will do a more thourough pass with tests.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/parsley/parsley.py (1)
13-15: Incorrect TypeVar usage in return type.
Tis an unboundTypeVarused in the return annotationdict[str, T], which provides no meaningful type information. The function returns a dictionary with mixed value types (strings, ints, floats, nested dicts), sodict[str, Any]is the appropriate type.This was flagged in a previous review but appears to still be present.
-from typing import List, Tuple, Union, Iterator, TypeVar +from typing import Any, List, Tuple, Union, Iterator -T = TypeVar("T") - -def parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, T]: +def parse_fields(bit_str: BitString, fields: List[Field]) -> dict[str, Any]:src/parsley/parse_to_object.py (1)
232-258: Generator method has incorrect return type annotation.
LoggerParser.parseusesyield(line 258), making it a generator. The return type should reflect this. This was flagged in a previous review.- def parse(self, buf: bytes, page_number: int) -> Union[ParsleyObject, ParsleyError]: + def parse(self, buf: bytes, page_number: int) -> Iterator[Union[ParsleyObject, ParsleyError]]:Note:
Iteratoris already imported at line 4.
🧹 Nitpick comments (4)
tests/test_parse_to_object.py (1)
185-196: Remove unused exception capture variables.The
as ecaptures are assigned but never used. Per static analysis (Ruff F841), these can be simplified.def test_parse_bitstring_empty(self): bit_str = BitString() #just an empty bitstring - with pytest.raises(IndexError) as e: + with pytest.raises(IndexError): BitstringParser().parse(bit_str) # message content is not asserted as an index error is expected def test_parse_bitstring_small(self): bit_str = BitString() bit_str.push(b'\xFF', 8) # only 8 bits, less than required 29 bits for SID - with pytest.raises(IndexError) as e: + with pytest.raises(IndexError): BitstringParser().parse(bit_str) # message content is not asserted as an index error is expectedsrc/parsley/parse_to_object.py (3)
87-92: Remove impossibleNonefrom return type.
_format_can_messagealways returns a tuple; theUnion[..., None]is misleading sinceNoneis never returned.@staticmethod - def _format_can_message(msg_sid: int, msg_data: List[int]) -> Union[Tuple[bytes, bytes], None]: + def _format_can_message(msg_sid: int, msg_data: List[int]) -> Tuple[bytes, bytes]:
213-225: Replace ambiguous en-dash characters with hyphen-minus.The docstring contains en-dash characters (
–) that should be regular hyphens (-) for consistency and to avoid encoding issues. Per static analysis (Ruff RUF001).""" Parse one logger record. Layout (little-endian unless stated): - 0 – 2 : ASCII 'L','O','G' + 0 - 2 : ASCII 'L','O','G' 3 : page number (uint8) - 4 – 12 : SID (uint32 LE) | timestamp (uint32 LE) | DLC (uint8) - 13 – .. : up to 8 bytes CAN payload + 4 - 12 : SID (uint32 LE) | timestamp (uint32 LE) | DLC (uint8) + 13 - .. : up to 8 bytes CAN payload -- ff-padding may follow, removed before parsing -- Returns whatever `format_can_message()` returns. Raises ValueError on any structural problem. """
174-174: Inconsistent Union syntax across parsers.
USBDebugParseruses the modern|syntax (ParsleyObject | ParsleyError) while other parsers useUnion[...]. Consider standardizing on one style for consistency.If using Python 3.10+, prefer
|everywhere:# Line 193 - def parse(self, frame: bytes) -> Union[ParsleyObject, ParsleyError]: + def parse(self, frame: bytes) -> ParsleyObject | ParsleyError: # Line 232 (after fixing Iterator issue) - def parse(self, buf: bytes, page_number: int) -> Iterator[Union[ParsleyObject, ParsleyError]]: + def parse(self, buf: bytes, page_number: int) -> Iterator[ParsleyObject | ParsleyError]: # Line 263 - def parse(self, bit_str: BitString) -> Union[ParsleyObject, ParsleyError]: + def parse(self, bit_str: BitString) -> ParsleyObject | ParsleyError:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/parsley/parse_to_object.py(1 hunks)src/parsley/parsley.py(2 hunks)tests/test_parse_to_object.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_parse_to_object.py (6)
src/parsley/bitstring.py (3)
BitString(1-49)push(30-41)pop(12-28)src/parsley/parse_to_object.py (14)
parse_to_object(113-162)_ParsleyParseInternal(23-162)ParsleyParser(164-169)USBDebugParser(171-188)LoggerParser(210-258)BitstringParser(260-266)parse(168-169)parse(174-188)parse(193-208)parse(232-258)parse(263-266)calculate_msg_bit_len(41-45)format_line(28-38)encode_data(48-66)src/parsley/parsley_message.py (2)
ParsleyError(14-23)ParsleyObject(25-49)tests/test_parsley.py (9)
test_parse(16-47)test_parse_partial_byte_fields(49-75)test_parse_sensor_analog(77-105)test_parse_bad_msg_type(107-111)test_parse_bitstring(161-170)test_calculate_msg_bit_length(200-204)test_format_line(206-221)test_encode_data(223-248)test_parse_usb_debug(250-257)tests/utils.py (2)
create_msg_sid_from_strings(11-24)approx(8-9)src/parsley/parsley.py (4)
parse(34-65)calculate_msg_bit_len(214-218)format_line(201-211)encode_data(175-193)
src/parsley/parse_to_object.py (4)
src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)src/parsley/bitstring.py (3)
BitString(1-49)push(30-41)pop(12-28)src/parsley/fields.py (4)
Field(6-36)Switch(181-197)Bitfield(199-232)get_fields(193-194)src/parsley/parse_utils.py (1)
hexify(1-13)
src/parsley/parsley.py (3)
src/parsley/bitstring.py (1)
BitString(1-49)src/parsley/fields.py (1)
Field(6-36)src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)
🪛 Ruff (0.14.8)
tests/test_parse_to_object.py
187-187: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
194-194: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
src/parsley/parse_to_object.py
177-177: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
197-197: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Avoid specifying long messages outside the exception class
(TRY003)
217-217: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
219-219: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
220-220: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
238-238: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Avoid specifying long messages outside the exception class
(TRY003)
252-252: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (14)
tests/test_parse_to_object.py (8)
1-16: LGTM - Well-organized imports and setup.The imports cover all necessary components for testing the new parsing architecture, including the internal parser classes, message types, and utilities.
19-23: Clean helper for normalizing test results.The
_to_dictmethod appropriately handles bothParsleyErrorandParsleyObjecttypes, making test assertions consistent.
109-114: Past review issue addressed.The test now correctly uses
isinstance(result, ParsleyError)as suggested in the previous review, and the'error' in result.errorcheck is valid since error messages are prefixed with "error:".
30-107: Comprehensive parsing tests with good coverage.The tests thoroughly cover successful parsing scenarios for
GENERAL_BOARD_STATUS,DEBUG_RAW, andSENSOR_ANALOGmessages, mirroring the patterns intest_parsley.pywhile validating the newParsleyObjectreturn type.
130-171: Good edge case coverage for invalid board IDs.The tests properly verify that invalid
board_type_idandboard_inst_idvalues fall back to hexified strings (e.g.,"0x1F") while still returning a validParsleyObject.
262-288: Solid USB debug parser test coverage.Tests cover normal parsing, empty data, invalid format (missing
$), and empty lines, ensuring proper error handling viaValueError.
290-365: Thorough logger parser tests.Good coverage including multi-message pages, wrong size, wrong signature, wrong page number, and empty pages. The buffer construction is well-documented with comments.
367-408: Complete live telemetry parser tests.Tests cover basic parsing with CRC validation, too-short frames, wrong header, and bad CRC scenarios.
src/parsley/parsley.py (1)
33-34: Deprecation decorator correctly applied.The
@deprecateddecorator is properly used to mark the oldparsefunction, directing users to the new class-based implementation. The typo from the previous review ("implementatio") has been fixed.src/parsley/parse_to_object.py (5)
1-22: Well-structured module header and imports.Good organization with clear module docstring, necessary imports, and module-level constants for formatting.
112-162: Core parsing logic looks correct.The
parse_to_objectmethod properly handles bothbytesandintSID inputs, decodes fields, and returns appropriateParsleyObjectorParsleyErrorresults. The docstring typo ("obejct") from the previous review has been fixed.
171-188: Good implementation of USBDebugParser.Clean parsing of ASCII USB-debug lines with proper error handling for invalid format.
190-208: Solid LiveTelemetryParser implementation.Proper frame validation (length, header, CRC) before parsing. The CRC8 validation is correctly implemented.
260-266: Clean BitstringParser implementation.Straightforward parsing of
BitStringobjects to extract SID and data.
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 (2)
src/parsley/parsley.py(2 hunks)tests/test_parse_to_object.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_parse_to_object.py (2)
src/parsley/parse_to_object.py (14)
parse_to_object(113-162)_ParsleyParseInternal(23-162)USBDebugParser(171-188)LiveTelemetryParser(190-208)LoggerParser(210-258)BitstringParser(260-266)parse(168-169)parse(174-188)parse(193-208)parse(232-258)parse(263-266)calculate_msg_bit_len(41-45)format_line(28-38)encode_data(48-66)src/parsley/parsley_message.py (2)
ParsleyError(14-23)ParsleyObject(25-49)
src/parsley/parsley.py (2)
src/parsley/bitstring.py (1)
BitString(1-49)src/parsley/fields.py (3)
Field(6-36)Switch(181-197)Bitfield(199-232)
🪛 Ruff (0.14.8)
tests/test_parse_to_object.py
187-187: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
194-194: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (8)
src/parsley/parsley.py (1)
31-31: Deprecation decorator applied correctly.The
@deprecateddecorator is properly applied with a clear migration message pointing users to the class-based implementation.tests/test_parse_to_object.py (7)
1-17: LGTM!The imports are well-organized and appropriate for the comprehensive test suite covering internal parsing logic, parser implementations, and data models.
18-28: LGTM!The
_to_dicthelper method provides a clean way to convert both success and error results to dictionaries for assertions, and the instantiation test correctly validates that the internal class cannot be instantiated.
30-58: LGTM!Comprehensive test that validates parsing of a complete GENERAL_BOARD_STATUS message, including timestamp encoding, bitfield manipulation, and field-by-field result verification.
109-155: LGTM!Excellent coverage of error cases including malformed message types, empty inputs, invalid SIDs, bad board IDs, and incomplete message data. The assertions correctly verify both error types and error recovery behavior (hexification of invalid board IDs).
262-288: LGTM!Comprehensive testing of the USB debug parser covering valid messages, empty data, format violations, and edge cases. The assertions correctly validate both successful parsing and appropriate error handling.
290-365: LGTM!Thorough testing of the logger parser with proper validation of the binary format, including multi-message pages, structural validation (size, signature, page number), and empty page handling. The tests correctly use struct packing and validate the expected parsing behavior.
367-409: LGTM!Comprehensive testing of the live telemetry parser including proper frame construction with CRC calculation, validation of length and header requirements, and CRC verification. The tests correctly exercise both success and error paths.
| def test_parse_bitstring_empty(self): | ||
| bit_str = BitString() #just an empty bitstring | ||
| with pytest.raises(IndexError) as e: | ||
| BitstringParser().parse(bit_str) | ||
| # message content is not asserted as an index error is expected | ||
|
|
||
| def test_parse_bitstring_small(self): | ||
| bit_str = BitString() | ||
| bit_str.push(b'\xFF', 8) # only 8 bits, less than required 29 bits for SID | ||
| with pytest.raises(IndexError) as e: | ||
| BitstringParser().parse(bit_str) | ||
| # message content is not asserted as an index error is expected |
There was a problem hiding this comment.
Remove unused exception info variables.
Lines 187 and 194 assign the exception info to e but never use it. Either remove the as e clause or add assertions on the exception message.
Apply this diff:
def test_parse_bitstring_empty(self):
bit_str = BitString() #just an empty bitstring
- with pytest.raises(IndexError) as e:
+ with pytest.raises(IndexError):
BitstringParser().parse(bit_str)
# message content is not asserted as an index error is expected
def test_parse_bitstring_small(self):
bit_str = BitString()
bit_str.push(b'\xFF', 8) # only 8 bits, less than required 29 bits for SID
- with pytest.raises(IndexError) as e:
+ with pytest.raises(IndexError):
BitstringParser().parse(bit_str)🧰 Tools
🪛 Ruff (0.14.8)
187-187: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
194-194: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In tests/test_parse_to_object.py around lines 185 to 196, the tests bind the
raised IndexError to a variable (`as e`) but never use it; remove the unused `as
e` clauses (change `with pytest.raises(IndexError) as e:` to `with
pytest.raises(IndexError):`) or alternately add explicit assertions against
`str(e.value)` if you intend to validate the exception message — update both
test_parse_bitstring_empty and test_parse_bitstring_small accordingly so no
unused variable remains.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ChrisYx511
left a comment
ChrisYx511
left a comment
There was a problem hiding this comment.
some final small changes! i think it's mainly just a typing mistake
ShreyShingala
force-pushed
the
Parse_Refactor
branch
from
4dbc6a7 to
7fd01c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parsley/parsley.py (1)
115-161: Incorrect return type for generator function.The
parse_loggerfunction usesyield(line 161), making it a generator. The return type annotation should reflect this.🔎 Proposed fix
+from typing import Any, Iterator -from typing import Any -def parse_logger(buf: bytes, page_number: int) -> tuple[bytes, bytes] | None: +def parse_logger(buf: bytes, page_number: int) -> Iterator[tuple[bytes, bytes]]:
🤖 Fix all issues with AI agents
In @src/parsley/parse_to_object.py:
- Around line 86-90: The return type annotation of format_can_message
incorrectly includes | None even though the function always returns a tuple of
bytes; update the function signature for format_can_message(msg_sid: int,
msg_data: list[int]) to return tuple[bytes, bytes] (remove the optional None),
and verify no callers or type hints elsewhere expect None so no other changes
are required.
- Around line 215-218: Replace the EN DASH (U+2013) characters with standard
hyphen-minus '-' in the docstring lines that currently read "0 – 2 : ASCII
'L','O','G'", "4 – 12 : SID (uint32 LE) | timestamp (uint32 LE) | DLC (uint8)"
and "13 – .. : up to 8 bytes CAN payload"; update those three ranges to use '-'
(e.g. "0 - 2 : ...", "4 - 12 : ...", "13 - .. : ...") and save the file with
UTF-8 encoding so the docstring only contains ASCII hyphens.
In @src/parsley/parsley_message.py:
- Around line 42-46: Both validators validate_msg_type and validate_msg_prio
incorrectly reject None even though the type hints allow None; update each
validator (validate_msg_type and validate_msg_prio in parsley_message.py) to
first check for value is None and return it (i.e., skip membership validation)
before testing membership against mt.msg_type / mt.msg_prio so None passes
validation and only non-None values are validated.
- Around line 36-40: The validator validate_msg_prio currently assumes a
non-null string and will raise when msg_prio is None; update validate_msg_prio
to skip validation when value is None (e.g., check `if value is not None and
value not in mt.msg_prio`) so optional msg_prio (typed str | None) is allowed,
then return value as before.
In @tests/test_parse_to_object.py:
- Line 187: Remove the unused exception variable in the pytest.raises context
managers: change usages of "with pytest.raises(IndexError) as e:" to "with
pytest.raises(IndexError):" (and do the same for the second occurrence at the
other location), since the captured exception variable `e` is never referenced.
🧹 Nitpick comments (6)
src/parsley/parsley_message.py (1)
14-14: Unused type parameter onParsleyError.
ParsleyErrordeclaresGeneric[T]but never uses the type variableTin its fields. This adds unnecessary complexity without providing type safety benefits.🔎 Proposed fix
-class ParsleyError(Generic[T]): +class ParsleyError: """Custom error container for Parsley errors."""tests/test_parsley.py (1)
12-12: Unused import:ParsleyError.The import of
ParsleyErroris not used in this test file. All tests still work with dict-based assertions (e.g.,'error' in res['data']at lines 112, 118, 124, 147). Consider removing this import or updating the tests to useParsleyErrorif that was the intention.tests/test_parsley_message.py (1)
35-54: Consider testingNonevalues for optional fields.The tests verify that invalid string values raise
ValidationErrorformsg_prioandmsg_type, but don't test whetherNone(which is allowed by the type annotations at lines 10-11 in parsley_message.py) is accepted or rejected. Add tests to ensureNoneis handled correctly by the validators.🔎 Suggested test additions
def test_parsley_object_none_prio(): """Test that None is valid for msg_prio if intended.""" obj = ParsleyObject( board_type_id='ANY', board_inst_id='GROUND', msg_prio=None, msg_type='RESET_CMD', data={} ) assert obj['msg_prio'] is None def test_parsley_object_none_type(): """Test that None is valid for msg_type if intended.""" obj = ParsleyObject( board_type_id='ANY', board_inst_id='GROUND', msg_prio='HIGH', msg_type=None, data={} ) assert obj['msg_type'] is Nonesrc/parsley/__init__.py (2)
24-47: Consider sorting__all__for maintainability.Static analysis suggests applying isort-style sorting to
__all__. While not critical, alphabetical ordering improves maintainability and reduces merge conflicts.🔎 Proposed sorted order
__all__ = [ "BitString", + "BitstringParser", + "LiveTelemetryParser", + "LoggerParser", + "ParsleyError", + "ParsleyObject", + "ParsleyParser", + "USBDebugParser", + "calculate_msg_bit_len", + "encode_data", "fields", + "format_line", "message_definitions", "message_types", - "ParsleyObject", - "ParsleyError", - "ParsleyParser", - "USBDebugParser", - "LiveTelemetryParser", - "LoggerParser", - "BitstringParser", - "parse_fields", "parse", + "parse_bitstring", "parse_board_type_id", "parse_board_inst_id", - "parse_bitstring", + "parse_fields", "parse_live_telemetry", - "parse_usb_debug", "parse_logger", - "format_line", - "encode_data", - "calculate_msg_bit_len", + "parse_usb_debug", ]
13-22: Pipeline warnings about partially unknown types in legacy parse functions.The linter reports that several functions from
parsley.pyhave partially unknown types (parse,parse_board_type_id,format_line,encode_data,calculate_msg_bit_len). While this doesn't block the current PR, consider adding type annotations to these legacy functions in a follow-up to improve type safety across the codebase.tests/test_parse_to_object.py (1)
114-114: Consider more specific error assertions.The assertions
assert 'error' in result.erroronly check if the substring "error" appears in the message. Since the error format is alwaysf"error: {error}"(line 151 in parse_to_object.py), this will always pass. Consider asserting specific error content or just checking that the error field is non-empty.Example improvement
# Instead of: assert 'error' in result.error # Consider: assert result.error # Non-empty check # Or check for specific error types: assert 'KeyError' in result.error or 'ValueError' in result.errorAlso applies to: 121-121, 128-128, 155-155
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.tomlsrc/parsley/__init__.pysrc/parsley/parse_to_object.pysrc/parsley/parsley.pysrc/parsley/parsley_message.pytests/test_parse_to_object.pytests/test_parsley.pytests/test_parsley_message.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_parsley_message.py (1)
src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)
tests/test_parsley.py (3)
src/parsley/parsley_message.py (1)
ParsleyError(14-23)tests/test_parse_to_object.py (1)
test_parse_empty(116-121)src/parsley/parsley.py (1)
parse(30-61)
src/parsley/parsley.py (3)
src/parsley/bitstring.py (2)
BitString(1-49)pop(12-28)src/parsley/fields.py (3)
Field(6-36)Switch(181-197)Bitfield(199-232)src/parsley/parse_to_object.py (3)
parse_fields(67-83)format_can_message(86-90)encode_data(46-64)
src/parsley/parse_to_object.py (5)
src/parsley/parsley_message.py (2)
ParsleyObject(25-49)ParsleyError(14-23)src/parsley/bitstring.py (3)
BitString(1-49)push(30-41)pop(12-28)src/parsley/fields.py (4)
Field(6-36)Switch(181-197)Bitfield(199-232)get_fields(193-194)src/parsley/parsley.py (4)
format_line(197-207)calculate_msg_bit_len(210-214)encode_data(171-189)parse(30-61)src/parsley/parse_utils.py (1)
hexify(1-13)
🪛 GitHub Actions: Run Lint
src/parsley/__init__.py
[warning] 14-14: Type of "parse" is partially unknown
[warning] 14-14: Type of "parse" is partially unknown
[warning] 14-14: Type of "parse_board_type_id" is partially unknown
[warning] 19-5: Type of "format_line" is partially unknown
[warning] 20-5: Type of "encode_data" is partially unknown
[warning] 21-5: Type of "calculate_msg_bit_len" is partially unknown
🪛 GitHub Check: Run Basedpyright
src/parsley/parse_to_object.py
[failure] 12-12:
Import "crc8" could not be resolved (reportMissingImports)
🪛 Ruff (0.14.10)
src/parsley/__init__.py
24-47: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
src/parsley/parse_to_object.py
175-175: Avoid specifying long messages outside the exception class
(TRY003)
193-193: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
215-215: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
217-217: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
218-218: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
236-236: Avoid specifying long messages outside the exception class
(TRY003)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
250-250: Avoid specifying long messages outside the exception class
(TRY003)
src/parsley/parsley_message.py
39-39: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_parse_to_object.py
187-187: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
194-194: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (10)
pyproject.toml (1)
21-21: No action needed. Pydantic 2.12.3 is a valid, secure version with no known vulnerabilities. The constraint>=2.12.3appropriately allows patch and minor updates within the v2.x series, including the latest 2.12.5.tests/test_parse_to_object.py (2)
25-28: LGTM - Well-designed test for static class enforcement.The test correctly verifies that the internal class cannot be instantiated and validates the error message content.
30-107: Excellent test coverage across parser implementations.The test suite comprehensively covers:
- Valid parsing scenarios with multiple field types (Numeric, Enum, ASCII, Bitfield)
- Error handling for malformed inputs
- Format-specific parsers (USB debug, logger, live telemetry)
- Edge cases (empty/minimal data, CRC validation, page number mismatches)
The use of realistic CAN message construction and proper type checking demonstrates thorough validation of the new parser infrastructure.
Also applies to: 262-409
src/parsley/parsley.py (3)
2-2: LGTM - Modern type annotations properly applied.The import additions and type annotation updates correctly use:
Anyfor heterogeneous dictionary values- Modern Python 3.9+ syntax (
list,dictinstead ofList,Dict)deprecateddecorator for the legacy functionAlso applies to: 9-9, 11-11
29-29: Deprecation decorator properly applied.The
@deprecateddecorator correctly marks the legacyparsefunction with an appropriate version and clear migration guidance.
157-157: LGTM - Helpful local variable type annotation.The explicit type annotation
list[int]improves code clarity and enables better static type checking.src/parsley/parse_to_object.py (4)
21-24: LGTM - Static-only class pattern correctly implemented.The
__init__guard properly prevents instantiation with a clear error message, enforcing the static-only design.
92-108: LGTM - Board ID parsing with fallback to hex representation.Both methods correctly handle unknown board IDs by falling back to hexified representation, enabling parsing to continue even with unrecognized hardware.
111-160: Well-structured parsing with comprehensive error handling.The
parse_to_objectmethod effectively:
- Accepts both bytes and integer SID formats for flexibility
- Extracts and decodes all message components systematically
- Gracefully handles decode failures by wrapping them in
ParsleyError- Returns strongly-typed results (
ParsleyObjectorParsleyError)The defensive initialization of variables and clear separation of success/error paths make the code maintainable.
12-12: No action required —crc8is properly declared as a dependency.The
crc8import is already specified in bothpyproject.toml(as"crc8>=0.2.1") andrequirements.txt. The static analysis warning is a false positive, likely due to the package not being installed in the CI environment.
| @field_validator("msg_type") | ||
| def validate_msg_type(cls, value): | ||
| if value not in mt.msg_type: | ||
| raise ValueError(f"Invalid msg_type type '{value}'") | ||
| return value |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the structure of the parsley_message.py file
head -60 src/parsley/parsley_message.pyRepository: waterloo-rocketry/parsley
Length of output: 1418
🏁 Script executed:
# Find the definition of mt and what msg_type contains
rg -n "msg_type" src/parsley/parsley_message.pyRepository: waterloo-rocketry/parsley
Length of output: 304
🏁 Script executed:
# Search for where mt is imported from and what it contains
rg -n "import.*mt\|from.*mt" src/parsley/Repository: waterloo-rocketry/parsley
Length of output: 51
🏁 Script executed:
# Find and examine the message_types.py file
find src/parsley -name "message_types.py" -o -name "*message_types*"Repository: waterloo-rocketry/parsley
Length of output: 97
🏁 Script executed:
# Look for msg_type definition in the repository
rg -n "msg_type\s*=" src/parsley/ --type=py -A 2Repository: waterloo-rocketry/parsley
Length of output: 3192
🏁 Script executed:
# Read the message_types.py file to see the full msg_type definition
cat src/parsley/message_types.pyRepository: waterloo-rocketry/parsley
Length of output: 7447
Validator doesn't handle None for optional msg_type.
MsgType is defined as str | None (line 11), but the validator at line 44 checks if value not in mt.msg_type without special-casing None. The msg_type dictionary in message_types.py contains only string keys (UNDEFINED, GENERAL_BOARD_STATUS, etc.) and does not include None. This means the validator will raise ValueError when None is passed, despite None being a valid value according to the type hint. The codebase confirms that None is an expected value (e.g., parse_to_object.py line 134 sets msg_type = None).
Add a check to skip validation for None values:
if value is not None and value not in mt.msg_type:
raise ValueError(f"Invalid msg_type type '{value}'")Apply the same fix to the validate_msg_prio validator (line 39), which has the identical issue.
🧰 Tools
🪛 Ruff (0.14.10)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/parsley/parsley_message.py around lines 42 - 46, Both validators
validate_msg_type and validate_msg_prio incorrectly reject None even though the
type hints allow None; update each validator (validate_msg_type and
validate_msg_prio in parsley_message.py) to first check for value is None and
return it (i.e., skip membership validation) before testing membership against
mt.msg_type / mt.msg_prio so None passes validation and only non-None values are
validated.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Not an issue, Both msg_prio and msg_type are only set to None at the beginning. Lowkey could've been a "" too but set to None to make it more clear to future developers that this is/will be changed. None never reaches parsley cause in the try loop there are only two potentials where they can't get replaced
-
msg_prio returns error at .decode() -> creates a new Parsley error which doesn't feature board priority and the msg_type which is a none gets hexified
-
msg_type eturns error at .decode() -> creates a new Parsley error and the msg_type gets hexified (the priority which would've been a normal string isn't included)
Could add a defensive fix, but idt its required
|
|
||
| def test_parse_bitstring_empty(self): | ||
| bit_str = BitString() #just an empty bitstring | ||
| with pytest.raises(IndexError) as e: |
There was a problem hiding this comment.
Remove unused exception variables.
The as e clause captures the exception but never uses it. Remove the unused variable binding.
🔎 Proposed fix
def test_parse_bitstring_empty(self):
bit_str = BitString() #just an empty bitstring
- with pytest.raises(IndexError) as e:
+ with pytest.raises(IndexError):
BitstringParser().parse(bit_str)
# message content is not asserted as an index error is expected
def test_parse_bitstring_small(self):
bit_str = BitString()
bit_str.push(b'\xFF', 8) # only 8 bits, less than required 29 bits for SID
- with pytest.raises(IndexError) as e:
+ with pytest.raises(IndexError):
BitstringParser().parse(bit_str)Also applies to: 194-194
🧰 Tools
🪛 Ruff (0.14.10)
187-187: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In @tests/test_parse_to_object.py at line 187, Remove the unused exception
variable in the pytest.raises context managers: change usages of "with
pytest.raises(IndexError) as e:" to "with pytest.raises(IndexError):" (and do
the same for the second occurrence at the other location), since the captured
exception variable `e` is never referenced.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/parsley/parsley_message.py:
- Around line 36-40: The validators validate_msg_prio and validate_msg_type
reject valid None values because they test membership in mt.msg_prio /
mt.msg_type without a None guard; update each validator (validate_msg_prio and
the counterpart validate_msg_type) to first allow None by only performing the
membership check when value is not None (e.g., if value is not None and value
not in mt.msg_prio / mt.msg_type) and otherwise return the value unchanged so
None is accepted.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/parsley/parsley_message.pytests/test_parsley.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_parsley.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/parsley/parsley_message.py
39-39: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
src/parsley/parsley_message.py (5)
1-4: LGTM!Imports are appropriate and well-organized for the functionality provided.
6-11: Type aliases are well-defined.The type aliases clearly express intent, and the union types
str | Nonecorrectly represent optional fields that will be validated at runtime.
13-23: LGTM!
ParsleyErroras a dataclass is appropriate for this error container, and the__getitem__method provides convenient dict-like access for backwards compatibility.
25-34: Class structure is sound.The use of
BaseModelwithGeneric[T]is a valid Pydantic pattern, and the genericdata: Tfield provides appropriate flexibility for various data types.
48-49: LGTM!The
__getitem__implementation usingmodel_dump()is clean and provides dict-like access for backwards compatibility.
| @field_validator("msg_prio") | ||
| def validate_msg_prio(cls, value): | ||
| if value not in mt.msg_prio: | ||
| raise ValueError(f"Invalid msg_prio type '{value}'") | ||
| return value |
There was a problem hiding this comment.
Validators reject valid None values.
Both validators have the critical issue already flagged in past review comments: they don't check for None before testing dictionary membership. Since MsgPrio and MsgType are typed as str | None, and the codebase does pass None values (confirmed in parse_to_object.py), these validators will incorrectly raise ValueError when None is provided.
Fix by adding a None check in each validator:
if value is not None and value not in mt.msg_prio: # or mt.msg_typeAlso applies to: 42-46
🧰 Tools
🪛 Ruff (0.14.10)
39-39: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/parsley/parsley_message.py around lines 36 - 40, The validators
validate_msg_prio and validate_msg_type reject valid None values because they
test membership in mt.msg_prio / mt.msg_type without a None guard; update each
validator (validate_msg_prio and the counterpart validate_msg_type) to first
allow None by only performing the membership check when value is not None (e.g.,
if value is not None and value not in mt.msg_prio / mt.msg_type) and otherwise
return the value unchanged so None is accepted.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Not an issue, Both msg_prio and msg_type are only set to None at the beginning. Lowkey could've been a "" too but set to None to make it more clear to future developers that this is/will be changed. None never reaches parsley cause in the try loop there are only two potentials where they can't get replaced
-
msg_prio returns error at .decode() -> creates a new Parsley error which doesn't feature board priority and the msg_type which is a none gets hexified
-
msg_type eturns error at .decode() -> creates a new Parsley error and the msg_type gets hexified (the priority which would've been a normal string isn't included)
Could add a defensive fix, but idt its required
ChrisYx511
left a comment
ChrisYx511
left a comment
There was a problem hiding this comment.
Fix nits / Explain. Otherwise lgtm!! I'd get someone else to review too :)
src/parsley/parsley.py
Outdated
|
|
||
| return res | ||
|
|
||
| @deprecated(version='2026.2', reason="This function is outdated, use the class implementation instead") |
There was a problem hiding this comment.
Link to the specific class that should be used, also deprecate the remainder (parse_usb_debug, etc.)
There was a problem hiding this comment.
Alright, will do! Also in another PR should I replace all of them so that they just use the object models under the hood because the code is mostly just copy pasted?
| def test_parse_bitstring_empty(self): | ||
| bit_str = BitString() #just an empty bitstring | ||
| with pytest.raises(IndexError) as e: | ||
| BitstringParser().parse(bit_str) | ||
| # message content is not asserted as an index error is expected | ||
|
|
||
| def test_parse_bitstring_small(self): | ||
| bit_str = BitString() | ||
| bit_str.push(b'\xFF', 8) # only 8 bits, less than required 29 bits for SID | ||
| with pytest.raises(IndexError) as e: | ||
| BitstringParser().parse(bit_str) | ||
| # message content is not asserted as an index error is expected |
There was a problem hiding this comment.
| @field_validator("msg_prio") | ||
| def validate_msg_prio(cls, value): | ||
| if value not in mt.msg_prio: | ||
| raise ValueError(f"Invalid msg_prio type '{value}'") | ||
| return value |
There was a problem hiding this comment.
| @field_validator("msg_type") | ||
| def validate_msg_type(cls, value): | ||
| if value not in mt.msg_type: | ||
| raise ValueError(f"Invalid msg_type type '{value}'") | ||
| return value |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/parsley/parsley.py`:
- Around line 121-122: The parse_logger function is a generator (it uses yield)
but its signature declares a non-iterator return type; change the signature of
parse_logger to return Iterator[tuple[bytes, bytes]] and add/import Iterator
from typing (e.g., from typing import Iterator) at the top of the file so the
annotation matches its generator behavior and remove the optional None from the
return type.
♻️ Duplicate comments (1)
src/parsley/parse_to_object.py (1)
230-256: Return type should beIteratorfor generator method.The
parsemethod usesyield(line 256), making it a generator. The return type annotation should reflect this.🔧 Proposed fix
Update the import on line 4:
-from typing import Any +from typing import Any, IteratorUpdate the method signature:
- def parse(self, buf: bytes, page_number: int) -> ParsleyObject | ParsleyError: + def parse(self, buf: bytes, page_number: int) -> Iterator[ParsleyObject | ParsleyError]:
🧹 Nitpick comments (2)
src/parsley/parse_to_object.py (1)
80-81: Redundant decode for Bitfield.Line 76 already assigns
res[field.name] = field.decode(data)for all fields. The Bitfield-specific block re-decodes with the same call, overwriting with an identical value.🧹 Suggested cleanup
if isinstance(field, Switch): nested_fields = field.get_fields(res[field.name]) res.update(_ParsleyParseInternal.parse_fields(bit_str, nested_fields)) - if isinstance(field, Bitfield): - res[field.name] = field.decode(data)src/parsley/parsley.py (1)
89-89: Unnecessary| Nonein return type annotations.Both
parse_live_telemetry(line 89) andparse_usb_debug(line 105) always returntuple[bytes, bytes]viaformat_can_message(). The| Noneis misleading since these functions never returnNone.Since these are deprecated, this is a minor nit.
🧹 Optional cleanup
-def parse_live_telemetry(frame: bytes) -> tuple[bytes, bytes] | None: +def parse_live_telemetry(frame: bytes) -> tuple[bytes, bytes]:-def parse_usb_debug(line: str) -> tuple[bytes, bytes] | None: +def parse_usb_debug(line: str) -> tuple[bytes, bytes]:Also applies to: 105-105
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/parsley/parse_to_object.pysrc/parsley/parsley.py
🧰 Additional context used
🪛 GitHub Check: Run Basedpyright
src/parsley/parse_to_object.py
[failure] 12-12:
Import "crc8" could not be resolved (reportMissingImports)
🪛 Ruff (0.14.11)
src/parsley/parse_to_object.py
175-175: Avoid specifying long messages outside the exception class
(TRY003)
193-193: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
236-236: Avoid specifying long messages outside the exception class
(TRY003)
239-239: Avoid specifying long messages outside the exception class
(TRY003)
250-250: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
src/parsley/parse_to_object.py (6)
92-160: LGTM!The parsing helpers and
parse_to_objectmethod are well-structured. Good handling of both integer and bytes input formsg_sid, and appropriate error handling that returnsParsleyErrorwith hex representations on parse failures.
162-167: LGTM!The abstract base class pattern is appropriate for defining the parser interface.
169-186: LGTM!Clean implementation that handles both SID-only and SID:data formats correctly.
188-206: LGTM!Proper validation of frame structure and CRC checksum before parsing.
258-264: LGTM!Clean implementation that extracts SID and data from the BitString before delegating to the internal parser.
12-12: No action required. Thecrc8dependency is properly declared inpyproject.toml(line 20) andrequirements.txt(line 2) ascrc8>=0.2.1. The static analysis warning was a false positive.Likely an incorrect or invalid review comment.
src/parsley/parsley.py (4)
9-12: LGTM!Proper deprecation decorator with clear migration path to the new internal implementation.
30-80: LGTM!Appropriate deprecation with clear migration guidance. Existing behavior is preserved for backwards compatibility.
170-198: LGTM!Correct return type annotations and proper deprecation with migration guidance.
206-225: LGTM!Proper deprecation with clear migration paths for the remaining utility functions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @deprecated(version='2026.2', reason="Deprecated; use LoggerParser.parse in the new LoggerParser object") | ||
| def parse_logger(buf: bytes, page_number: int) -> tuple[bytes, bytes] | None: |
There was a problem hiding this comment.
Generator function needs Iterator return type.
parse_logger uses yield (line 168), making it a generator. The return type should be Iterator[tuple[bytes, bytes]] instead of tuple[bytes, bytes] | None.
🔧 Proposed fix
Update imports:
-from typing import Any
+from typing import Any, IteratorUpdate signature:
-def parse_logger(buf: bytes, page_number: int) -> tuple[bytes, bytes] | None:
+def parse_logger(buf: bytes, page_number: int) -> Iterator[tuple[bytes, bytes]]:🤖 Prompt for AI Agents
In `@src/parsley/parsley.py` around lines 121 - 122, The parse_logger function is
a generator (it uses yield) but its signature declares a non-iterator return
type; change the signature of parse_logger to return Iterator[tuple[bytes,
bytes]] and add/import Iterator from typing (e.g., from typing import Iterator)
at the top of the file so the annotation matches its generator behavior and
remove the optional None from the return type.
|
Also solved #95 |
|
and #76 |
4 participants
Created a new parsley_message.py file containing two new classes: ParsleyError and ParsleyObject.
ParsleyError class stores error details and is created when an error occurs.
ParsleyObject class stores the parsed can message to replace the old dict.
Added tests in test_parse_to_object.py. Tests are basically copy pasted from test_parse, but ported over to work for classes. Main difference is parse_to_object always returns a parsley_object which is already converts everything to the decoded string (only returning the hex in an error), while in parsley the functions returned bytes themselves (SID/data) so tests were edited to account for this.
This change is
Summary by CodeRabbit
New Features
Refactor
Deprecations
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.