Fix for: Build should stop if npm fails #4513 #4556
Conversation
WalkthroughThe pull request modifies the build UI script to enhance its error handling and control flow. The script now uses double quotes for importing the environment module and adds the Changes
✨ Finishing Touches
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: 0
🧹 Nitpick comments (2)
pio-scripts/build_ui.py (2)
8-8: Consider using a more explicit no-op command.Instead of
"null", consider using a more standard no-op command like":"(colon) which is a built-in shell no-op command.- exitCode = env.Execute("null") + exitCode = env.Execute(":")🧰 Tools
🪛 Ruff (0.8.2)
8-8: Undefined name
env(F821)
7-7: Consider extracting ANSI color codes into constants.The ANSI color codes are duplicated and could be hard to maintain. Consider extracting them into named constants at the top of the file.
# Add at the top of the file ERROR_STYLE = '\x1b[0;31;43m' SUCCESS_STYLE = '\x1b[6;33;42m' RESET_STYLE = '\x1b[0m' # Then use them in the messages print(ERROR_STYLE + 'Node.js is not installed...' + RESET_STYLE)Also applies to: 12-12, 20-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/build_ui.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pio-scripts/build_ui.py
1-1: Undefined name Import
(F821)
8-8: Undefined name env
(F821)
13-13: Undefined name env
(F821)
16-16: Undefined name env
(F821)
🔇 Additional comments (2)
pio-scripts/build_ui.py (2)
1-2: LGTM! Good use ofshutilfor executable checking.Using
shutil.which()is the recommended way to check for executables in the system PATH.🧰 Tools
🪛 Ruff (0.8.2)
1-1: Undefined name
Import(F821)
10-21: LGTM! Good error handling and user feedback.The implementation successfully:
- Runs npm commands in the correct order
- Checks for failures and aborts the build when needed
- Provides helpful error messages with documentation links
🧰 Tools
🪛 Ruff (0.8.2)
13-13: Undefined name
env(F821)
16-16: Undefined name
env(F821)
Fix for: Build should stop if npm fails #4513
Fixes #4513
This will check if Node,js is present in path and will continue to build WLED
or it will cancel the build and show error
Summary by CodeRabbit