Add XML command "GetLifeSpan" #575
Conversation
WalkthroughThe changes introduce a new command, Changes
Possibly related PRs
Suggested labels
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: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deebot_client/commands/xml/init.py (2 hunks)
- deebot_client/commands/xml/life_span.py (1 hunks)
- deebot_client/events/init.py (1 hunks)
- tests/commands/xml/test_life_span.py (1 hunks)
Additional context used
Ruff
tests/commands/xml/test_life_span.py
16-16:
deebot_client.events.base.Eventimported but unusedRemove unused import:
deebot_client.events.base.Event(F401)
Additional comments not posted (6)
deebot_client/commands/xml/__init__.py (3)
12-12: LGTM!The import statement for
GetLifeSpanis correctly added and aligns with the PR objective.
24-24: LGTM!Adding
GetLifeSpanto the__all__list is necessary and aligns with the PR objective.
32-32: LGTM!Adding
GetLifeSpanto the_COMMANDSlist is necessary and aligns with the PR objective.tests/commands/xml/test_life_span.py (2)
19-29: LGTM!The parameterized test function is well-structured and covers different component types and expected events. The use of helper functions
get_request_xmlandassert_commandmakes the test function readable and maintainable.
32-44: LGTM!The parameterized test function is well-structured and covers different error scenarios. The use of helper functions
get_request_xmlandassert_commandmakes the test function readable and maintainable.deebot_client/events/__init__.py (1)
143-143: LGTM!The addition of the
DUST_CASE_HEAPenumeration value to theLifeSpanclass is consistent with the existing naming convention and enhances the functionality of the class by providing an additional state related to the dust case component. The code change is well-integrated and aligns with the purpose of theLifeSpanclass.
flubshi
force-pushed
the
xml_life_span
branch
2 times, most recently
from
9fb46b1 to
8461c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deebot_client/commands/xml/init.py (2 hunks)
- deebot_client/commands/xml/life_span.py (1 hunks)
- deebot_client/events/init.py (1 hunks)
- tests/commands/xml/test_life_span.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- deebot_client/commands/xml/init.py
- deebot_client/commands/xml/life_span.py
- deebot_client/events/init.py
Additional context used
Ruff
tests/commands/xml/test_life_span.py
3-3:
typing.TYPE_CHECKINGimported but unusedRemove unused import:
typing.TYPE_CHECKING(F401)
Additional comments not posted (2)
tests/commands/xml/test_life_span.py (2)
16-26: LGTM!The parameterized test function
test_get_life_spanis well-structured and covers different component types and their corresponding lifespan events. The test cases use meaningful parameter names and values, making the test easy to understand. Great job!
29-41: LGTM!The parameterized test function
test_get_life_span_errorcovers two error scenarios for theGetLifeSpancommand. The test cases use meaningful parameter names and values, making the test easy to understand. Well done!
flubshi
force-pushed
the
xml_life_span
branch
from
8461c1d to
e080b85
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
deebot_client/commands/xml/__init__.py (1)
Line range hint
1-45: Consider standardizing the case convention between JSON and XML commands.The PR objectives mention different naming conventions between JSON (lowercase) and XML (uppercase) for lifespan types. Consider implementing a case conversion utility or standardizing the convention across both formats to improve maintainability and reduce confusion.
Potential approaches:
- Create a shared enum for component types with methods to convert case as needed
- Implement case conversion in the command handlers
- Standardize on one case format throughout the codebase
Would you like assistance in implementing any of these approaches?
tests/commands/xml/test_life_span.py (2)
14-25: Consider adding edge cases and documentation.While the current test cases cover the basic functionality well, consider enhancing the test coverage with:
- Edge cases:
- Zero values for
leftandtotal- Maximum possible values
- Invalid values (negative numbers)
- Documentation explaining the percentage calculation logic (e.g.,
left/total * 100)Example additional test cases:
# Add to the parametrize list: ("Brush", LifeSpan.BRUSH, 0, 100, LifeSpanEvent(LifeSpan.BRUSH, 0, 0)), ("SideBrush", LifeSpan.SIDE_BRUSH, 200, 200, LifeSpanEvent(LifeSpan.SIDE_BRUSH, 100, 100)),
27-39: Enhance error handling test coverage.Consider adding more error test cases to cover:
- Invalid component type
- Invalid numeric values in
leftortotalattributes- Malformed XML structure
Example additional test cases:
# Add to the parametrize list: "<ctl ret='ok' type='InvalidComponent' left='100' total='100'/>", "<ctl ret='ok' type='Brush' left='invalid' total='100'/>", "<ctl ret='ok' type='Brush' left='100' total='0'/>", "<malformed>xml</malformed>",deebot_client/commands/xml/life_span.py (1)
18-27: Document the type conversion convention in class docstring.While the code comments explain the type conversion logic, this important convention should be documented in the class docstring for better visibility and maintainability.
Apply this diff to enhance the documentation:
class GetLifeSpan(XmlCommandWithMessageHandling): - """GetLifeSpan command.""" + """GetLifeSpan command. + + Note: + There's a naming convention difference between JSON and XML commands: + - JSON component types start with lowercase (e.g., 'brushSide') + - XML component types start with uppercase (e.g., 'BrushSide') + """deebot_client/events/__init__.py (1)
143-143: Consider refactoring to handle JSON/XML casing differences.Based on the PR objectives, there's a need to handle different casing between JSON (lowercase) and XML (uppercase) component types. Instead of hardcoding the string values, consider implementing a more flexible solution.
Here's a suggested approach:
@unique class LifeSpan(str, Enum): """Enum class for all possible life span components.""" + def __str__(self) -> str: + """Return the JSON representation (lowercase).""" + return self.value + + def to_xml(self) -> str: + """Return the XML representation (uppercase first letter).""" + return self.value[0].upper() + self.value[1:] BRUSH = "brush" FILTER = "heap" # ... other values ... DUST_CASE_HEAP = "dustCaseHeap"This approach:
- Maintains backward compatibility for JSON
- Adds XML support through a new method
- Centralizes the casing logic
- Makes it easier to modify the transformation if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
deebot_client/commands/xml/__init__.py(2 hunks)deebot_client/commands/xml/life_span.py(1 hunks)deebot_client/events/__init__.py(1 hunks)tests/commands/xml/test_life_span.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
deebot_client/commands/xml/life_span.py (3)
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:45-46
Timestamp: 2024-11-10T18:57:05.372Z
Learning: When calculating percentages, prefer using `percent = round((left / total) * 100, 2)` over f-strings like `percent = f"{left / total:.2%}"` for better human readability.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:39-43
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In type conversion, only the first character should be changed to lower or upper case, preserving the rest of the string as is.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:23-26
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In Python, `str.capitalize()` uppercases the first character and lowercases the rest of the string, which may not be suitable when the rest of the string's casing should remain unchanged.
🔇 Additional comments (6)
deebot_client/commands/xml/__init__.py (2)
12-12: LGTM! Import statement is properly placed and follows conventions.
The import follows the established pattern and maintains alphabetical ordering.
24-24: LGTM! Command properly exported in all.
The command is correctly added to the public API in alphabetical order.
tests/commands/xml/test_life_span.py (2)
1-12: LGTM! Clean and well-organized imports.
The imports are properly organized and all are being utilized in the test cases.
17-19: Consider testing XML/JSON type conversion.
Based on the PR objectives, there's a known difference in naming conventions between JSON (lowercase) and XML (uppercase) component types. Consider adding test cases that explicitly verify this conversion logic to ensure consistent behavior.
deebot_client/commands/xml/life_span.py (1)
1-16: LGTM! Well-structured imports and type hints.
The file follows best practices with proper type hints, conditional imports for type checking, and clear module documentation.
deebot_client/events/__init__.py (1)
143-143: LGTM! The new enum value follows the established pattern.
The addition of DUST_CASE_HEAP = "dustCaseHeap" maintains consistency with existing enum values in terms of naming convention and string value format.
flubshi
force-pushed
the
xml_life_span
branch
2 times, most recently
from
63237c2 to
4a8ac96
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #575 +/- ##
==========================================
+ Coverage 86.44% 86.54% +0.10%
==========================================
Files 96 97 +1
Lines 3555 3590 +35
Branches 310 314 +4
==========================================
+ Hits 3073 3107 +34
+ Misses 428 426 -2
- Partials 54 57 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
deebot_client/api_client.py (2)
26-33: LGTM! Consider enhancing the class documentation.The dataclass is well-structured with proper type hints and immutability.
Consider expanding the class documentation to describe the purpose of each list:
@dataclass(frozen=True) class Devices: - """Devices.""" + """Container for categorized device lists. + + Attributes: + mqtt: List of modern devices supporting MQTT protocol + xmpp: List of legacy devices using XMPP protocol + not_supported: List of devices that are not currently supported + """
81-91: Consider explicit error handling for static_device_info retrieval.While the current error handling is functional, consider catching and logging specific exceptions from
get_static_device_infoto aid in debugging.- if static_device_info := await get_static_device_info( - device["class"] - ): + try: + if static_device_info := await get_static_device_info( + device["class"] + ): + mqtt.append(DeviceInfo(device, static_device_info)) + else: + _LOGGER.warning( + 'Device class "%s" not recognized. Please add support for it: %s', + device["class"], + device, + ) + not_supported.append(device) + except Exception as e: + _LOGGER.error( + 'Failed to get static device info for class "%s": %s', + device["class"], + str(e), + ) + not_supported.append(device)deebot_client/hardware/deebot/4vhygi.py (1)
Line range hint
88-94: IncludeDUST_CASE_HEAPin lifespan capabilitiesTo fully support the new
GetLifeSpancommand for theDUST_CASE_HEAPcomponent, please includeLifeSpan.DUST_CASE_HEAPin thetypestuple and update thegetmethod accordingly in thelife_spancapabilities.Apply this diff to include the new lifespan component:
life_span=CapabilityLifeSpan( - types=(LifeSpan.BRUSH, LifeSpan.FILTER, LifeSpan.SIDE_BRUSH), + types=(LifeSpan.BRUSH, LifeSpan.FILTER, LifeSpan.SIDE_BRUSH, LifeSpan.DUST_CASE_HEAP), event=LifeSpanEvent, - get=[GetLifeSpan([LifeSpan.BRUSH, LifeSpan.FILTER, LifeSpan.SIDE_BRUSH])], + get=[GetLifeSpan([LifeSpan.BRUSH, LifeSpan.FILTER, LifeSpan.SIDE_BRUSH, LifeSpan.DUST_CASE_HEAP])], reset=ResetLifeSpan, ),deebot_client/hardware/deebot/__init__.py (1)
Line range hint
29-38: Add logging when device capabilities are missingThe function
get_static_device_infonow returnsNoneif no device information is found, but it does not log this event. Adding a warning log will help in debugging when a device class is not recognized.Apply this diff to add a warning message:
if device := DEVICES.get(class_): _LOGGER.debug("Capabilities found for %s", class_) return device + _LOGGER.warning("No capabilities found for class %s", class_) return NoneREADME.md (1)
50-50: Verify example code completenessThe device initialization has been updated to use
devices_.mqtt[0]. However, the example should:
- Include error handling for empty device list
- Document the mqtt attribute's purpose
- bot = Device(devices_.mqtt[0], authenticator) + if not devices_.mqtt: + raise ValueError("No MQTT-capable devices found") + + # Initialize the first MQTT-capable device + bot = Device(devices_.mqtt[0], authenticator)tests/conftest.py (1)
130-132: Consider parameterizing the device IDThe hardcoded device ID "yna5xi" makes the test less flexible. Consider:
- Moving it to a constants file
- Making it configurable through a fixture parameter
@pytest.fixture -async def static_device_info() -> StaticDeviceInfo: - info = await get_static_device_info("yna5xi") +async def static_device_info(device_id: str = "yna5xi") -> StaticDeviceInfo: + info = await get_static_device_info(device_id) assert info is not None return infoThe assertion for non-None info is a good addition as it fails fast and provides clear error messages.
tests/hardware/test_init.py (2)
95-95: Improved null safety in device info handlingGood improvement in error handling:
- Explicit None return for unknown devices
- Added null check before accessing capabilities
However, consider adding test cases for more edge cases.
Add test cases for:
@pytest.mark.parametrize( ("class_", "expected"), [ ("not_specified", lambda: None), ("yna5xi", lambda: DEVICES["yna5xi"]), ("invalid_device", lambda: None), # Add this case ("", lambda: None), # Add this case ], )Also applies to: 239-241
234-234: Consider parameterizing device IDsThe test IDs are hardcoded in the
idsparameter. Consider deriving them from the test cases to avoid maintenance issues.- ids=["5xu9h3", "itk04l", "yna5xi", "p95mgv"], + ids=lambda val: val[0], # Use the class_ parameter as the test ID.github/workflows/python-publish.yml (2)
32-34: Consider using a Python script for version updatesWhile
sedworks, a Python script would be more robust for handling the version update, especially if the TOML structure changes.- run: | - sed -i "s/^version = \".*\"/version = \"${{ github.event.release.tag_name }}\"/" pyproject.toml + run: | + python -c ' + import toml + import sys + config = toml.load("pyproject.toml") + config["project"]["version"] = "${{ github.event.release.tag_name }}" + toml.dump(config, open("pyproject.toml", "w")) + '
Add documentation for UV publish configuration and credentials
The workflow uses
uv publishbut lacks documentation about required PyPI credentials. The environment configuration shows PyPI integration but needs documentation for:
- Required PyPI token configuration in GitHub environment secrets
- UV publish behavior and credentials handling
Locations needing documentation:
README.md: Add a section about publishing and release process.github/workflows/python-publish.yml: Add inline comments explaining the environment setup🔗 Analysis chain
Line range hint
36-43: Document UV publish configurationThe switch to
uv publishsimplifies the workflow, but ensure PyPI credentials are properly configured in the environment.
Let me check the content of README.md to see if it contains UV publish configuration documentation.
Let me check if there's any UV-specific configuration or documentation in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for PyPI configuration documentation find . -type f -exec grep -l "PyPI" {} \; | grep -i "readme\|docs"Length of output: 81
Script:
#!/bin/bash # Check README.md content for UV publish configuration cat README.md | grep -A 5 -B 5 "PyPI"Length of output: 537
Script:
#!/bin/bash # Search for UV configuration or documentation rg -i "uv|pypi" -A 5 -B 5 .github/workflows/python-publish.ymlLength of output: 995
🛑 Comments failed to post (1)
.pre-commit-config.yaml (1)
10-10:
⚠️ Potential issueReconsider using Python 3.13
Python 3.13 is currently in development and not yet released for production use. Consider using Python 3.12 which is the latest stable version.
- python: python3.13 + python: python3.12📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.python: python3.12
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
deebot_client/commands/xml/life_span.py (1)
23-26: Add input validation for life_span parameter.Consider adding validation to ensure the life_span parameter is not None and contains a valid value.
def __init__(self, life_span: LifeSpan) -> None: + if not life_span: + raise ValueError("life_span parameter is required") # type for JSON commands starts with small letter, while XML types start with upper letter. Workaround: xml_type = life_span.value[0].upper() + life_span.value[1:] super().__init__({"type": xml_type})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
deebot_client/commands/xml/__init__.py(2 hunks)deebot_client/commands/xml/common.py(0 hunks)deebot_client/commands/xml/life_span.py(1 hunks)deebot_client/events/__init__.py(1 hunks)tests/commands/xml/test_life_span.py(1 hunks)
💤 Files with no reviewable changes (1)
- deebot_client/commands/xml/common.py
🚧 Files skipped from review as they are similar to previous changes (3)
- deebot_client/events/init.py
- tests/commands/xml/test_life_span.py
- deebot_client/commands/xml/init.py
🧰 Additional context used
📓 Learnings (1)
deebot_client/commands/xml/life_span.py (3)
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:45-46
Timestamp: 2024-11-10T18:57:05.372Z
Learning: When calculating percentages, prefer using `percent = round((left / total) * 100, 2)` over f-strings like `percent = f"{left / total:.2%}"` for better human readability.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:39-43
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In type conversion, only the first character should be changed to lower or upper case, preserving the rest of the string as is.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:23-26
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In Python, `str.capitalize()` uppercases the first character and lowercases the rest of the string, which may not be suitable when the rest of the string's casing should remain unchanged.
🔇 Additional comments (2)
deebot_client/commands/xml/life_span.py (2)
1-16: LGTM! Imports and type checking are well organized.
The imports are properly structured with type checking conditional imports and clear organization.
45-46:
Add division by zero protection.
The percentage calculation needs protection against division by zero:
- percent = round((left / total) * 100, 2)
+ percent = round((left / total) * 100, 2) if total > 0 else 0.0Likely invalid or redundant comment.
flubshi
force-pushed
the
xml_life_span
branch
4 times, most recently
from
148683a to
07c2f14
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
deebot_client/commands/xml/life_span.py (1)
40-43: Remove redundant null check.The null check at line 42 is redundant since we already verify "type" exists in xml.attrib at line 36.
- component_type = xml.attrib.get("type") - if component_type is None: - return HandlingResult.analyse() + component_type = xml.attrib.get("type")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: deebot_client/commands/xml/life_span.py#L42
Added line #L42 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
deebot_client/commands/xml/common.py(1 hunks)deebot_client/commands/xml/life_span.py(1 hunks)tests/commands/xml/test_life_span.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deebot_client/commands/xml/common.py
- tests/commands/xml/test_life_span.py
🧰 Additional context used
📓 Learnings (1)
deebot_client/commands/xml/life_span.py (3)
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:45-46
Timestamp: 2024-11-10T18:57:05.372Z
Learning: When calculating percentages, prefer using `percent = round((left / total) * 100, 2)` over f-strings like `percent = f"{left / total:.2%}"` for better human readability.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:39-43
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In type conversion, only the first character should be changed to lower or upper case, preserving the rest of the string as is.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:23-26
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In Python, `str.capitalize()` uppercases the first character and lowercases the rest of the string, which may not be suitable when the rest of the string's casing should remain unchanged.
🪛 GitHub Check: codecov/patch
deebot_client/commands/xml/life_span.py
[warning] 42-42: deebot_client/commands/xml/life_span.py#L42
Added line #L42 was not covered by tests
🔇 Additional comments (4)
deebot_client/commands/xml/life_span.py (4)
1-16: LGTM! Well-organized imports section.
The imports are properly structured with type checking conditional and necessary dependencies.
18-27: LGTM! Type conversion logic aligns with established patterns.
The initialization correctly handles the case conversion between JSON and XML formats, maintaining the casing of subsequent characters as per previous learnings.
50-50:
Add validation for life span type.
The LifeSpan enum creation could fail with invalid types. Add validation to handle this gracefully.
- event_bus.notify(LifeSpanEvent(LifeSpan(xml_type), percent, left))
+ try:
+ life_span = LifeSpan(xml_type)
+ event_bus.notify(LifeSpanEvent(life_span, percent, left))
+ except ValueError:
+ return HandlingResult.analyse()Likely invalid or redundant comment.
46-48:
Add error handling for numeric conversions.
The int conversions could raise ValueError if the XML attributes contain non-numeric values.
- left = int(xml.attrib.get("left", 0))
- total = int(xml.attrib.get("total", 0))
+ try:
+ left = int(xml.attrib.get("left", 0))
+ total = int(xml.attrib.get("total", 0))
+ except ValueError:
+ return HandlingResult.analyse()Likely invalid or redundant comment.
flubshi
force-pushed
the
xml_life_span
branch
from
07c2f14 to
673ff64
Compare
@edenhaus I fixed everything except for one mypy issue. Could you please help here? Feel free to change directly |
flubshi
force-pushed
the
xml_life_span
branch
from
673ff64 to
b4d4028
Compare
|
@edenhaus CI is happy now. Only failing code quality job seems unrelated, because dev is also affected. What are the next steps to merge? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
deebot_client/commands/xml/life_span.py (2)
18-27: Extract type conversion logic into a helper method.The type conversion logic for handling the casing difference between JSON and XML types appears in both
__init__and_handle_xml. Consider extracting this into a helper method to avoid duplication and improve maintainability.class GetLifeSpan(XmlCommandWithMessageHandling): """GetLifeSpan command.""" name = "GetLifeSpan" + @staticmethod + def _convert_type_case(type_str: str, to_upper: bool = True) -> str: + """Convert type string case while preserving the rest of the string. + + Args: + type_str: The type string to convert + to_upper: If True, converts first char to upper, else to lower + """ + if not type_str: + return type_str + return (type_str[0].upper() if to_upper else type_str[0].lower()) + type_str[1:] + def __init__(self, life_span: LifeSpan) -> None: - # type for JSON commands starts with small letter, while XML types start with upper letter. Workaround: - xml_type = life_span.value[0].upper() + life_span.value[1:] + xml_type = self._convert_type_case(life_span.value, to_upper=True) super().__init__({"type": xml_type})
41-43: Remove redundant null check.The null check at line 42 is redundant since we already verify the presence of "type" in xml.attrib at line 36. The
get()method will never return None for an existing attribute.- component_type = xml.attrib.get("type") - if component_type is None: - return HandlingResult.analyse() + component_type = xml.attrib.get("type")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: deebot_client/commands/xml/life_span.py#L42
Added line #L42 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
deebot_client/commands/xml/__init__.py(2 hunks)deebot_client/commands/xml/common.py(1 hunks)deebot_client/commands/xml/life_span.py(1 hunks)deebot_client/events/__init__.py(1 hunks)tests/commands/xml/test_life_span.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deebot_client/events/init.py
- tests/commands/xml/test_life_span.py
- deebot_client/commands/xml/common.py
- deebot_client/commands/xml/init.py
🧰 Additional context used
📓 Learnings (1)
deebot_client/commands/xml/life_span.py (3)
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:45-46
Timestamp: 2024-11-10T18:57:05.372Z
Learning: When calculating percentages, prefer using `percent = round((left / total) * 100, 2)` over f-strings like `percent = f"{left / total:.2%}"` for better human readability.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:39-43
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In type conversion, only the first character should be changed to lower or upper case, preserving the rest of the string as is.
Learnt from: flubshi
PR: DeebotUniverse/client.py#575
File: deebot_client/commands/xml/life_span.py:23-26
Timestamp: 2024-11-10T18:57:05.372Z
Learning: In Python, `str.capitalize()` uppercases the first character and lowercases the rest of the string, which may not be suitable when the rest of the string's casing should remain unchanged.
🪛 GitHub Check: codecov/patch
deebot_client/commands/xml/life_span.py
[warning] 42-42: deebot_client/commands/xml/life_span.py#L42
Added line #L42 was not covered by tests
🔇 Additional comments (3)
deebot_client/commands/xml/life_span.py (3)
1-16: LGTM!
The imports and type checking setup are well-organized and follow best practices.
50-50:
Add error handling for LifeSpan enum creation.
The creation of LifeSpan enum could fail if the converted XML type doesn't match any enum value.
46-48:
Add error handling for integer conversion.
The integer conversion of left and total attributes could raise ValueError if the values are not valid integers.
- left = int(xml.attrib.get("left", 0))
- total = int(xml.attrib.get("total", 0))
- percent: float = round((left / total) * 100, 2) if total > 0 else 0.0
+ try:
+ left = int(xml.attrib.get("left", 0))
+ total = int(xml.attrib.get("total", 0))
+ percent: float = round((left / total) * 100, 2) if total > 0 else 0.0
+ except ValueError:
+ return HandlingResult.analyse()Likely invalid or redundant comment.
|
Will review it in the next days :) |
edenhaus
force-pushed
the
xml_life_span
branch
from
05d4bbd to
b8351f5
Compare
2 participants
Adds GetLifeSpan command.
Compared to the JSON commands, this allows to request only one component_type per request.
JSON life span types always start with a lower letter, while XML compnent type always starts capitalized. I am currently not really happy with my approach of converting these types.
Runtime tested:
Summary by CodeRabbit
New Features
GetLifeSpan, for handling life span-related XML messages.DUST_CASE_HEAP, to the existingLifeSpanenum.Bug Fixes
GetLifeSpancommand with improved response validation.Tests
GetLifeSpancommand to ensure proper functionality and error handling.Refactor
has_sub_elementmethod to be an instance-level property in theXmlCommandclass.