Skip to content
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 "-T option null" issue #162

Closed

Conversation

unverbraucht
Copy link
Contributor

Summary

When #144 was merged I missed a check to see if device_topic_suffix is null. This causes issues #155 and #159 . This PR adds a simple config check and fixes the issue for me when I don't add this argument.

Alternatives Considered

There are other PRs that also fix this issue but this is a simple one-liner. I also ported the changes to config.json from the next folder over to the main autodiscovery config.json so these options work once next is merged.

Testing Steps

  1. Run next without setting a value for device_topic_suffix
  2. Add-On should start up

@catduckgnaf
Copy link

Nah, they want to wait months for "upstream" to fix it instead.
mine was a one liner too.

I decided to spend effort on actually making an integration that people want.

@unverbraucht
Copy link
Contributor Author

Nah, they want to wait months for "upstream" to fix it instead. mine was a one liner too.

I decided to spend effort on actually making an integration that people want.

This has not been my experience with this project.

Since I was the person introducing the bug (sorry about that) I think it's fair that I fix it :)

@unverbraucht
Copy link
Contributor Author

@catduckgnaf since you also fixed the issue in your branch (along many improvements, cudos!) could you review this PR?

@catduckgnaf
Copy link

I can't approve changes here. :(
Thanks for the "new" motivation to get my but into gear with the integration. And for the compliment!

I will have 2 repos, one for the "integration" and one for the add-on.

I am waiting to see if the current maintainers want me to take over the add-on....

@benklop
Copy link
Contributor

benklop commented Oct 27, 2023

This is actually caused by the path to the image being incorrect: #164 is probably the right fix

@deviantintegral
Copy link
Collaborator

Agreed with going with #164. Closing this one for now, but if someone thinks we'll need both changes then feel free to reopen.

@unverbraucht
Copy link
Contributor Author

Absolutely fine with the other fix if it fixes things :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants