-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
deprecate module currentweather and weatherforecast #2433
Conversation
buxxi
commented
Jan 23, 2021
- Adding logging when using deprecated weather-modules (had to add a NodeHelper for each)
- Updating sample config file to use the new weather module
- Added comment in code about not accepting new features
- Added warning in the old weather modules readme
Codecov Report
@@ Coverage Diff @@
## develop #2433 +/- ##
========================================
Coverage 39.40% 39.40%
========================================
Files 20 20
Lines 1609 1609
========================================
Hits 634 634
Misses 975 975
Continue to review full report at Codecov.
|
Thanks. I like this. Might give some issues with your other weather PR. Which one would you like me to merge first? |
The other one, this one is easier to redo :) (should only be the CHANGELOG that was changed in both?) |
Merged the other one. Is the weatherEndpoint still necessary in the config? |
Yes, both before and after the other pull request. Could possibly make it so it makes sane guesses for weatherEndpoint based on type when it's not specified? I'm not really sure what the meaning is for configuring the specific endpoints, I would just have mapped each type to one endpoint. But then there's a fourth for paying users which does exactly what? |
I think indeed it's a good idea to use a default one that works no matter what type of account you have. The simpler the necessary config, the better. |
Removed the default value for weatherEndpoint and if it's not set have sane defaults. |
Thanks for your great contribution! |