use f-strings and walrus operators #15
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThe PR refactors plugin.py to adopt Pythonic idioms—replacing string concatenation and .format calls with f-strings, leveraging walrus operators and dict.get for concise JSON and parameter handling—privatizes helper functions, centralizes magic values, bumps the plugin version, and adds a new time-delay device to measure latency. Sequence diagram for Time Delay device update on new datasequenceDiagram
participant Domoticz
participant QuattPlugin
participant Devices
participant time
Domoticz->>QuattPlugin: onMessage(Connection, Data)
QuattPlugin->>QuattPlugin: _process_response(data)
QuattPlugin->>time: time.time_ns()
QuattPlugin->>Devices: _update_device(unit=35, sValue=delay, nValue=0)
Devices-->>QuattPlugin: Update device 35 with delay
Class diagram for refactored QuattPlugin and helper functionsclassDiagram
class QuattPlugin {
httpConn
sendAfterConnect
discovered_data
__init__()
onStart()
onConnect(Connection, Status, Description)
onStop()
onHeartbeat()
onMessage(Connection, Data)
onDisconnect(Connection)
}
class _dump_config_to_log {
}
class _create_basic_devices {
}
class _create_dynamic_devices {
}
class _process_response {
}
class _update_device {
}
QuattPlugin -- _dump_config_to_log : uses
QuattPlugin -- _create_basic_devices : uses
QuattPlugin -- _create_dynamic_devices : uses
QuattPlugin -- _process_response : uses
QuattPlugin -- _update_device : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @etmmvdp - I've reviewed your changes - here's some feedback:
- The
_process_responsefunction is quite long and repetitive—consider defining a mapping of JSON keys to device units and update parameters, then looping over it to reduce boilerplate and improve maintainability. - Many of the helper functions are defined at the module level with leading underscores; you might encapsulate them as private methods on
QuattPluginor move them to a separate utility module to avoid polluting the global namespace. - You’re using
time.time_ns()for the delay calculation, which requires Python 3.7+—verify that the Domoticz environment supports this or provide a fallback likeint(time.time() * 1000)for compatibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_process_response` function is quite long and repetitive—consider defining a mapping of JSON keys to device units and update parameters, then looping over it to reduce boilerplate and improve maintainability.
- Many of the helper functions are defined at the module level with leading underscores; you might encapsulate them as private methods on `QuattPlugin` or move them to a separate utility module to avoid polluting the global namespace.
- You’re using `time.time_ns()` for the delay calculation, which requires Python 3.7+—verify that the Domoticz environment supports this or provide a fallback like `int(time.time() * 1000)` for compatibility.
## Individual Comments
### Comment 1
<location> `plugin.py:318` </location>
<code_context>
+ _update_device(self, 25, round(value, 1), 1)
+
+ # Time Delay
+ if (cic_time := data.get("time")) is not None:
+ if (cic_ts := cic_time.get("ts")) is not None:
+ cic_ts_millis = cic_ts
+ system_ts_millis = time.time_ns() // 1_000_000
+ delay = system_ts_millis - cic_ts_millis
+ _update_device(self, 35, delay, 0)
+
except Exception as e:
</code_context>
<issue_to_address>
The calculation of delay assumes synchronized clocks and may be misleading.
Document the assumption of synchronized clocks or add a mechanism to handle clock drift or offsets to ensure accurate delay reporting.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Time Delay
+ if (cic_time := data.get("time")) is not None:
+ if (cic_ts := cic_time.get("ts")) is not None:
+ cic_ts_millis = cic_ts
+ system_ts_millis = time.time_ns() // 1_000_000
+ delay = system_ts_millis - cic_ts_millis
+ _update_device(self, 35, delay, 0)
=======
# Time Delay
# Assumes system clock and device clock are synchronized.
# If clocks are not synchronized, delay calculation may be inaccurate.
# TODO: Implement clock drift/offset correction if device and system clocks can differ.
if (cic_time := data.get("time")) is not None:
if (cic_ts := cic_time.get("ts")) is not None:
cic_ts_millis = cic_ts
system_ts_millis = time.time_ns() // 1_000_000
delay = system_ts_millis - cic_ts_millis
_update_device(self, 35, delay, 0)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+317
to
+323
| # Time Delay | ||
| if (cic_time := data.get("time")) is not None: | ||
| if (cic_ts := cic_time.get("ts")) is not None: | ||
| cic_ts_millis = cic_ts | ||
| system_ts_millis = time.time_ns() // 1_000_000 | ||
| delay = system_ts_millis - cic_ts_millis | ||
| _update_device(self, 35, delay, 0) |
There was a problem hiding this comment.
suggestion: The calculation of delay assumes synchronized clocks and may be misleading.
Document the assumption of synchronized clocks or add a mechanism to handle clock drift or offsets to ensure accurate delay reporting.
Suggested change
| # Time Delay | |
| if (cic_time := data.get("time")) is not None: | |
| if (cic_ts := cic_time.get("ts")) is not None: | |
| cic_ts_millis = cic_ts | |
| system_ts_millis = time.time_ns() // 1_000_000 | |
| delay = system_ts_millis - cic_ts_millis | |
| _update_device(self, 35, delay, 0) | |
| # Time Delay | |
| # Assumes system clock and device clock are synchronized. | |
| # If clocks are not synchronized, delay calculation may be inaccurate. | |
| # TODO: Implement clock drift/offset correction if device and system clocks can differ. | |
| if (cic_time := data.get("time")) is not None: | |
| if (cic_ts := cic_time.get("ts")) is not None: | |
| cic_ts_millis = cic_ts | |
| system_ts_millis = time.time_ns() // 1_000_000 | |
| delay = system_ts_millis - cic_ts_millis | |
| _update_device(self, 35, delay, 0) |
Comment on lines
+126
to
+131
| Domoticz.Debug(f"Device: {x} - {Devices[x]}") | ||
| Domoticz.Debug(f"Device ID: '{Devices[x].ID}'") | ||
| Domoticz.Debug(f"Device Name: '{Devices[x].Name}'") | ||
| Domoticz.Debug(f"Device nValue: {Devices[x].nValue}") | ||
| Domoticz.Debug(f"Device sValue: '{Devices[x].sValue}'") | ||
| Domoticz.Debug(f"Device LastLevel: {Devices[x].LastLevel}") |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
| Domoticz.Device(Name=name, Unit=unit, TypeName=type_name, Options=options[0] if options else {}, Image=options[1] if len(options) > 1 else 0).Create() | ||
|
|
||
| def createDynamicDevices(self, data): | ||
| def _create_dynamic_devices(self, data): |
There was a problem hiding this comment.
issue (code-quality): Low code quality found in _create_dynamic_devices - 19% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| Domoticz.Device(Name=name, Unit=unit, TypeName=type_name, Options=options[0] if options else {}, Image=options[1] if len(options) > 1 else 0).Create() | ||
|
|
||
| def processResponse(self, data): | ||
| def _process_response(self, data): |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Low code quality found in _process_response - 13% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
|
It seems to be a big change but that's mostly using new python features without changing functionality. Let me know if you like it or not. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Modernize the Quatt Domoticz plugin by refactoring for Pythonic style, consolidating helper functions, bumping version, and adding a time delay device.
New Features:
Enhancements: