-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix automation describeCondition for number state type #21052
Fix automation describeCondition for number state type #21052
Conversation
When using a number state condition in an automation, the UI used an incorrect evaluation when trying to describe the condition which made the label default to the default value. To fix this, I just changed the evaluation to check directly for `undefined` value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @koostamas
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
WalkthroughThe update to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range comments (7)
src/data/automation_i18n.ts (7)
Line range hint
56-56
: Use template literals for string concatenation.The static analysis tool has flagged several instances where string concatenation is used. It's recommended to use template literals for better readability and maintainability.
- "Error in describing trigger: " + error.message + `Error in describing trigger: ${error.message}` - "Error in describing condition: " + error.message + `Error in describing condition: ${error.message}`Also applies to: 80-80, 712-712
Line range hint
74-74
: Specify a more specific type thanany
.Using
any
type disables TypeScript's static type checking, which can lead to runtime errors that are hard to debug. Consider specifying a more explicit type for error handling.- catch (error: any) { + catch (error: Error) {Also applies to: 706-706
Line range hint
261-261
: Avoid shadowing global properties.The variable name
toString
shadows a global property, which can lead to confusion and errors. Consider renaming this variable.- let toString = ""; + let toDescription = "";
Line range hint
401-401
: UseNumber.parseInt
instead of the globalparseInt
.To align with modern JavaScript practices and ensure consistency, use
Number.parseInt
instead of the globalparseInt
.- parseInt(trigger.seconds.substring(1)) + Number.parseInt(trigger.seconds.substring(1))Also applies to: 402-402, 430-430, 431-431, 467-467, 468-468
Line range hint
405-405
: UseNumber.isNaN
instead ofisNaN
.The global
isNaN
function can lead to unexpected results due to type coercion. UseNumber.isNaN
for a more reliable check.- isNaN(seconds) + Number.isNaN(seconds)Also applies to: 434-434, 471-471
Line range hint
327-327
: Avoid non-null assertions.Non-null assertions can lead to runtime errors if the assumptions about non-nullability prove incorrect. Review the logic to ensure that the values are indeed non-null or handle potential null cases explicitly.
Also applies to: 1002-1002, 1013-1013
Line range hint
91-91
: Refactor to reduce complexity.The functions
tryDescribeTrigger
andtryDescribeCondition
are flagged for excessive complexity. Consider refactoring to simplify these functions, possibly by breaking them down into smaller, more focused functions.Also applies to: 723-723
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just release the draft status from your PR and it will be ready to merge. Thank you for your contribution!
Proposed change
When using a number state condition in an automation, the UI used an incorrect evaluation when trying to describe the condition which made the label default to the default value:
To fix this, I just changed the evaluation to check directly for
undefined
value.(The red rectangles are just for directing attention, edited on the screenshot, not in the code (obviously)).
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
undefined
values, ensuring more reliable functionality.