Add option to hide weather errors (empty output) #375
Conversation
📝 WalkthroughWalkthroughThreads a new tmux option Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TM as tmux / scripts/dracula.sh
participant Wrapper as scripts/weather_wrapper.sh
participant Weather as scripts/weather.sh
TM->>Wrapper: invoke weather plugin (show_fahrenheit, show_location, weather_hide_errors, fixed_location)
Wrapper->>Weather: call main(show_fahrenheit, show_location, location, hide_errors)
alt Weather fetch succeeds
Weather-->>Wrapper: weather output
Wrapper-->>TM: render weather output
else Timeout or fetch failure
alt hide_errors = false
Weather-->>Wrapper: "Weather Unavailable" / "Unknown Location"
Wrapper-->>TM: render error message
else hide_errors = true
Weather-->>Wrapper: (no output)
Wrapper-->>TM: (no weather output rendered)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 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. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/weather_wrapper.sh (1)
14-17: Update function documentation to reflect the new parameter.The documentation lists only three arguments, but the function now accepts four parameters after adding
_hide_errors. Update the comment block to include the new parameter.🔎 Apply this diff to update the documentation:
# Arguments: # show fahrenheit, either "true" (default) or "false" # show location, either "true" (default) or "false" +# hide errors, either "true" or "false" (default) # optional fixed location to query data about, e.g. "Houston, Texas"scripts/weather.sh (1)
93-96: Update function documentation to reflect the new parameter.The documentation lists only three arguments, but the function now accepts four parameters. Update the comment block to include the new
hide_errorsparameter.🔎 Apply this diff to update the documentation:
# Arguments # show fahrenheit, either "true" (default) or "false" # show location, either "true" (default) or "false" # optional fixed location to query data about, e.g. "Houston, Texas" +# hide errors, either "true" or "false" (default)
🧹 Nitpick comments (1)
scripts/weather.sh (1)
118-121: LGTM! Consider simplifying the empty output.The error suppression logic is correct with the early return preventing subsequent error messages. The implementation aligns well with the PR objectives.
💡 Optional simplification
Since
printf ''outputs nothing, you could simplify by removing it:if "$_hide_errors"; then - printf '' return fiThis is purely stylistic and doesn't affect functionality.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/dracula.sh(2 hunks)scripts/weather.sh(1 hunks)scripts/weather_wrapper.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/dracula.sh (1)
scripts/utils.sh (1)
get_tmux_option(3-12)
🔇 Additional comments (6)
scripts/weather_wrapper.sh (2)
19-23: LGTM!The argument handling correctly receives the new
_hide_errorsparameter and maintains proper ordering for all four parameters.
30-30: LGTM!The invocation correctly passes all four parameters to
weather.shin the expected order: show_fahrenheit, show_location, location, hide_errors.scripts/dracula.sh (2)
21-21: LGTM!The new option is correctly retrieved with an appropriate default value of
false, ensuring backward compatibility.
314-314: LGTM!The weather wrapper invocation correctly passes the new
weather_hide_errorsparameter in the proper position, and thefixed_locationremains properly quoted to handle location names with spaces.scripts/weather.sh (2)
98-102: LGTM!The parameter handling correctly receives and assigns the
_hide_errorsflag with an appropriate default value offalse.
106-110: LGTM!The timeout error handling correctly suppresses the error message when
_hide_errorsis true, enabling the empty output behavior described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/weather.sh(1 hunks)scripts/weather_wrapper.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/weather.sh
🔇 Additional comments (1)
scripts/weather_wrapper.sh (1)
31-31: Argument reordering is intentional but depends on verification.The wrapper receives arguments in the order (fahrenheit, location_toggle, hide_errors, location) but calls weather.sh with (fahrenheit, location_toggle, location, hide_errors). This reordering maintains location as weather.sh's 3rd parameter while adding hide_errors as a new 4th parameter, which appears to preserve weather.sh's backward compatibility.
This implementation is correct if:
- All callers of weather_wrapper.sh pass arguments in the new order
- weather.sh correctly accepts hide_errors as the 4th parameter
The verification script in the previous comment will confirm both conditions.
1 participant
@dracula-weather-hide-errorsfor theweatherplugin.set -g @dracula-show-empty-plugins falseto hide the weather segment entirely when it can’t be fetched.Config example:
No more "Weather Unavailable":