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

homebridge-ambient-weather-sensors #611

Closed
peledies opened this issue Dec 5, 2023 · 22 comments
Closed

homebridge-ambient-weather-sensors #611

peledies opened this issue Dec 5, 2023 · 22 comments
Labels
verified use when a plugin meets the criteria - adds the verified badge text

Comments

@peledies
Copy link

peledies commented Dec 5, 2023

Link To GitHub Repo

https://github.com/peledies/homebridge-ambient-weather-sensors

Link To NPM Package

https://www.npmjs.com/package/homebridge-ambient-weather-sensors

Plugin Icon (Optional)

No response

@peledies peledies added the pending the label given to a new verification/icon request label Dec 5, 2023
Copy link

github-actions bot commented Dec 5, 2023

✅ Pre-checks completed successfully.

@peledies
Copy link
Author

peledies commented Dec 7, 2023

/check

Copy link

github-actions bot commented Dec 7, 2023

✅ Pre-checks completed successfully.

@bwp91 bwp91 added currently-reviewing use for starting a review - adds a comment with the verification list with empty checkboxes and removed pending the label given to a new verification/icon request labels Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

  • - The plugin must successfully install.
  • - The plugin must implement the Homebridge Plugin Settings GUI.
  • - The plugin must not start unless it is configured.
  • - The plugin must be of type dynamic platform.
  • - The plugin must not offer the same nor less functionality than that of any existing verified plugin.
  • - The plugin must not execute post-install scripts that modify the user's system in any way.
  • - The plugin must not contain any analytics or calls that enable you to track the user.
  • - The plugin must not throw unhandled exceptions, the plugin must catch and log its own errors.
  • - The plugin must be published to npm and the source code available on GitHub.
  • - A GitHub release should be created for every new version of your plugin, with patch notes.
  • - The plugin must run on all Active LTS versions of Node.js, at the time of writing this is Node v18 and v20.
  • - The plugin must not require the user to run Homebridge in a TTY or with non-standard startup parameters, even for initial configuration.
  • - If the plugin needs to write files to disk (cache, keys, etc.), it must store them inside the Homebridge storage directory.



Comment /check to run checks again.

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

One comment per thought of mine 😄

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

Screenshot 2023-12-08 at 21 51 07

maybe these should be placeholder values rather than defaults? could be a pain for a user on a mobile to need to select (all) and delete initial values

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/70a111b108d75b6a1ca692e676e519940e6b18bc/src/platform.ts#L14

What happens to the cache file and the naming system if a user has more than once instance of your plugin running?

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

Here you are awaiting your fetchDevices fn

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/70a111b108d75b6a1ca692e676e519940e6b18bc/src/platform.ts#L120

but this function may throw an error here

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/70a111b108d75b6a1ca692e676e519940e6b18bc/src/platform.ts#L115C15-L115C15

which would be unhandled since it is ultimately not wrapped around any try/catch.

@peledies
Copy link
Author

peledies commented Dec 8, 2023

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/70a111b108d75b6a1ca692e676e519940e6b18bc/src/platform.ts#L14

What happens to the cache file and the naming system if a user has more than once instance of your plugin running?

good point, I had not considered a user having multiple instances of the plugin running

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

could you base the cache file name on the api key? seems like the only unique difference there could be between multiple instances

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

The criteria that I am concerned about is the not throwing unhandled errors with regards to this comment:

#611 (comment)

I can't reproduce the case since I don't have valid credentials but I think this could end up throwing errors

@peledies
Copy link
Author

peledies commented Dec 8, 2023

@bwp91 Thanks for the feedback I have made all the suggested changes and published version v1.1.2.

Let me know if you see anything else.

@peledies
Copy link
Author

peledies commented Dec 8, 2023

/check

@github-actions github-actions bot added pending the label given to a new verification/icon request and removed currently-reviewing use for starting a review - adds a comment with the verification list with empty checkboxes labels Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

✅ Pre-checks completed successfully.

@bwp91
Copy link
Contributor

bwp91 commented Dec 8, 2023

No matter how much error catching you do, if you end up throwing an error in the catch block, this will crash homebridge.

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/6ddcff8e76013c44edafbc8423d7937d1174aa4c/src/platform.ts#L125

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/6ddcff8e76013c44edafbc8423d7937d1174aa4c/src/platform.ts#L208

I picked up two cases here in the catch block where the error is thrown.

Instead of throwing the error (string message) this should be written to the homebridge log as a warning/error.

I hope this makes sense

@peledies
Copy link
Author

peledies commented Dec 9, 2023

No matter how much error catching you do, if you end up throwing an error in the catch block, this will crash homebridge.

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/6ddcff8e76013c44edafbc8423d7937d1174aa4c/src/platform.ts#L125

https://github.com/peledies/homebridge-ambient-weather-sensors/blob/6ddcff8e76013c44edafbc8423d7937d1174aa4c/src/platform.ts#L208

I picked up two cases here in the catch block where the error is thrown.

Instead of throwing the error (string message) this should be written to the homebridge log as a warning/error.

I hope this makes sense

I see what you mean. those have now been replaced with error logging

@peledies
Copy link
Author

peledies commented Dec 9, 2023

/check

@bwp91 bwp91 added the reviewed label Dec 9, 2023
Copy link

github-actions bot commented Dec 9, 2023

  • General
    • The plugin must be of type dynamic platform.
    • The plugin must not offer the same nor less functionality than that of any existing verified plugin.
  • Repo
    • The plugin must be published to NPM and the source code available on a GitHub repository, with issues enabled.
    • A GitHub release should be created for every new version of your plugin, with release notes.
  • Environment
    • The plugin must run on all supported LTS versions of Node.js, at the time of writing this is Node v18 and v20.
    • The plugin must successfully install and not start unless it is configured.
    • The plugin must not execute post-install scripts that modify the users' system in any way.
    • The plugin must not require the user to run Homebridge in a TTY or with non-standard startup parameters, even for initial configuration.
  • Codebase
    • The plugin must implement the Homebridge Plugin Settings GUI.
    • The plugin must not contain any analytics or calls that enable you to track the user.
    • If the plugin needs to write files to disk (cache, keys, etc.), it must store them inside the Homebridge storage directory.
    • The plugin must not throw unhandled exceptions, the plugin must catch and log its own errors.

Everything Looks Good!

@bwp91 bwp91 added verified use when a plugin meets the criteria - adds the verified badge text and removed pending the label given to a new verification/icon request reviewed labels Dec 9, 2023
Copy link

github-actions bot commented Dec 9, 2023

Congratulations! Your plugin has been verified.

You can now add one of the Verified by Homebridge badges to your plugin's README:

verified-by-homebridge

[![verified-by-homebridge](https://badgen.net/badge/homebridge/verified/purple)](https://github.com/homebridge/homebridge/wiki/Verified-Plugins)

verified-by-homebridge

[![verified-by-homebridge](https://img.shields.io/badge/homebridge-verified-blueviolet?color=%23491F59&style=for-the-badge&logoColor=%23FFFFFF&logo=homebridge)](https://github.com/homebridge/homebridge/wiki/Verified-Plugins)

Your plugin is now also eligible to display a ❤️ Donate button on its tile in the Homebridge UI. See https://github.com/homebridge/homebridge/wiki/Donation-Links for instructions.

If for any reason in the future you can no longer maintain your plugin, please consider transferring it to our unmaintained plugins repo. We can take ownership until another willing developer comes along.

Don't forget to join the official Homebridge Discord server, where plugin developers can get tips and advice from other developers and the Homebridge project team in the #plugin-development channel!

Thank you for your contribution to the Homebridge Community.
https://homebridge.io

@peledies peledies closed this as completed Dec 9, 2023
@cytan299
Copy link

cytan299 commented Jan 21, 2024

Hi Peledies
Thanks for writing the ambient weather plugin. I've installed your plugin and it's working great reading my temperature and humidity sensors! When do you think you'd have the wind speed, wind direction and relative pressure added to the plugin? I think adding these sensor values will make your plugin even more useful.

Thanks again!

cytan

@peledies
Copy link
Author

Hi Peledies Thanks for writing the ambient weather plugin. I've installed your plugin and it's working great reading my temperature and humidity sensors! When do you think you'd have the wind speed, wind direction and relative pressure added to the plugin? I think adding these sensor values will make your plugin even more useful.

Thanks again!

cytan

@cytan299 Thanks for your interest in the plugin. if you could open an issue on the repo for the plugin it will help me keep track of the feature requests. https://github.com/peledies/homebridge-ambient-weather-sensors

@cytan299
Copy link

cytan299 commented Jan 22, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verified use when a plugin meets the criteria - adds the verified badge text
Projects
None yet
Development

No branches or pull requests

3 participants