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.
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
Add Gotify component #53050
Add Gotify component #53050
Changes from all commits
1c0b66a
4581a82
fee256f
a4bc041
4532ed1
135ff81
283d95e
5c610f3
e60f4d0
c53b8a5
537b537
99825bc
02654ad
f4bd2b5
d3e02cb
3ee71d2
ca2cf85
632f84e
29ef3f8
eaa96e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The string,
notify
, should be imported fromhomeassistant.const.Platform
.Eg.
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.
Now the standard pattern here is
Again, what you have probably works, but I'm not sure if updating to the new methods is required for merging.
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.
Same as above the notify platform doesn't support config entries.
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.
Yea, makes sense.
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.
I think this needs moving outside of this
if
statement (or thewhile
loop needs moving inside of it), as otherwise this function errors out if thegotify
domain doesn't exist.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.
Yea, this is a bug for sure. Since the lower half of the function doesn't do anything really if
entry_names
is empty, I'd recommend a short circuit exit.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.
As this is a dictionary and you only seem to care about the values, you can instead iterate over
hass.data[DOMAIN].values()
directly.You can also simplify this using list comprehension and even avoid empty list initialization.
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.
This will cause an infinite loop if the same name exists since you never remove the name from the
entry_names
list.You're probably better off filtering by name in the list above and then using it's length here.
That said, we're not using the actual name values at all, so it could be simplified further.
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.
It looks like this is only used in one block below. It can go within the
if CONF_TOKEN in user_input:
block.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.
You could drop
not (CONF_TOKEN in user_input)
and only checknot return_token
here asreturn_token
is initialized as a falsy value.