Device release aware update #1646
Conversation
WalkthroughThis pull request introduces three main changes. The upgrade example no longer hardcodes the WLED IP address; instead, it accepts a command-line argument by importing the Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 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: 1
🧹 Nitpick comments (2)
src/wled/wled.py (1)
638-642: Improve code style by removing unnecessary parentheses.The parentheses around the if condition are not needed in Python.
- if(self._device.info.release is not None): + if self._device.info.release is not None: update_file = f"{self._device.info.brand}_{version}_{self._device.info.release}.bin{gzip}" else: update_file = f"WLED_{version}_{architecture}{ethernet}.bin{gzip}"src/wled/models.py (1)
428-430: Fix typo and enhance docstring.The docstring has a typo and could benefit from more comprehensive examples.
release: str | None = None - """The releae name, e.g ESP32_Ethernet, ESP8266_160""" + """The release name of the WLED device. + + Examples: + - ESP32_Ethernet + - ESP8266_160 + - ESP8266_compat + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/upgrade.py(2 hunks)src/wled/models.py(1 hunks)src/wled/wled.py(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/upgrade.py (1)
10-15: Improve function docstring.The docstring should be moved to the correct position and enhanced to better describe the function's purpose and usage.
Apply this diff to improve the docstring:
-async def main() -> None: - if len(sys.argv) < 2: - print("Usage: python upgrade.py <ip_address>") - sys.exit(1) - - """Show example on upgrade your WLED device.""" +async def main() -> None: + """Demonstrate WLED device upgrade functionality. + + This example script connects to a WLED device using the provided IP address, + checks for the latest stable release, and performs an upgrade while maintaining + device-specific release compatibility. + + Usage: + python upgrade.py <ip_address> + """ + if len(sys.argv) < 2: + print("Usage: python upgrade.py <ip_address>") + sys.exit(1)🧰 Tools
🪛 Ruff (0.8.2)
10-10: Missing docstring in public function
(D103)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/upgrade.py(2 hunks)src/wled/models.py(1 hunks)src/wled/wled.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/wled/models.py
- src/wled/wled.py
🧰 Additional context used
🪛 Ruff (0.8.2)
examples/upgrade.py
10-10: Missing docstring in public function
(D103)
🔇 Additional comments (3)
examples/upgrade.py (3)
5-5: LGTM!The
sysmodule import is correctly placed and necessary for command-line argument handling.
11-14: LGTM!The error handling for command-line arguments has been implemented as suggested, with a clear usage message and proper exit code.
25-25: LGTM!The WLED client instantiation now correctly uses the IP address from command-line arguments, making the script more flexible.
| if len(sys.argv) < 2: | ||
| print("Usage: python upgrade.py <ip_address>") | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
??
If we wanted to do proper arg handling in the example, we'd do it differently. Let's remove it.
There was a problem hiding this comment.
Forgive my inexperience with python, if there is a better way please suggest rather than rejecting an improvement
| @@ -17,7 +22,7 @@ async def main() -> None: | |||
| print("No stable version found") | |||
| return | |||
|
|
|||
| async with WLED("10.10.11.54") as led: | |||
| async with WLED(sys.argv[1]) as led: | |||
There was a problem hiding this comment.
let's remove this is just an example
There was a problem hiding this comment.
It's easier to run the example to test the behaviour against many different devices in you don't have to edit the file by hand reach each find
src/wled/models.py
Outdated
| """The releae name, e.g ESP32_Ethernet, ESP8266_160""" | ||
| """The release name of the WLED device. | ||
|
|
||
| Examples: | ||
| - ESP32_Ethernet | ||
| - ESP8266_160 | ||
| - ESP8266_compat | ||
| """ |
There was a problem hiding this comment.
Why are there two comment sections?
I'm not understanding the comment contents, why is this relevant?
There was a problem hiding this comment.
Ah, well spotted. The alternative format suggestion by CodeRabbit hasn't applied properly
src/wled/wled.py
Outdated
| if self._device.info.release is not None: | ||
| update_file = f"{self._device.info.brand}_{version}_{self._device.info.release}.bin{gzip}" | ||
| else: | ||
| update_file = f"WLED_{version}_{architecture}{ethernet}.bin{gzip}" | ||
|
|
There was a problem hiding this comment.
This can be rewritten without the else:.
|
Happy with the changes now? |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
github-actions
bot
added
the
stale
|
Are you happy with the changes I made @frenck ? |
github-actions
bot
removed
the
stale
Proposed Changes
Where the WLED device is new enough to return the release name as part of the device info, use this value to build the bin file name.
When this release info is not present, fallback to the original behaviour to download the "vanilla" build for your architecture
This is so that is the device is for example running the ESP8266_160 or ESP8266_compat release they stay on that release rather than switching to the default ESP8266 build
Summary by CodeRabbit