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

deprecate module currentweather and weatherforecast #2433

Merged
merged 4 commits into from
Jan 24, 2021
Merged

deprecate module currentweather and weatherforecast #2433

merged 4 commits into from
Jan 24, 2021

Conversation

buxxi
Copy link
Contributor

@buxxi 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-io
Copy link

codecov-io commented Jan 23, 2021

Codecov Report

Merging #2433 (0683734) into develop (925113f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2433   +/-   ##
========================================
  Coverage    39.40%   39.40%           
========================================
  Files           20       20           
  Lines         1609     1609           
========================================
  Hits           634      634           
  Misses         975      975           
Impacted Files Coverage Δ
modules/default/currentweather/currentweather.js 1.40% <ø> (ø)
modules/default/weatherforecast/weatherforecast.js 22.96% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 925113f...0683734. Read the comment docs.

@MichMich
Copy link
Collaborator

Thanks. I like this. Might give some issues with your other weather PR. Which one would you like me to merge first?

@buxxi
Copy link
Contributor Author

buxxi commented Jan 23, 2021

The other one, this one is easier to redo :)

(should only be the CHANGELOG that was changed in both?)

@MichMich
Copy link
Collaborator

Merged the other one. Is the weatherEndpoint still necessary in the config?

@buxxi
Copy link
Contributor Author

buxxi commented Jan 23, 2021

Yes, both before and after the other pull request.
Using forecast/daily will just get stuck on "Loading..." without specifying it, that's why I defined it like that in the sample config.

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?

@MichMich
Copy link
Collaborator

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.

@buxxi
Copy link
Contributor Author

buxxi commented Jan 24, 2021

Removed the default value for weatherEndpoint and if it's not set have sane defaults.

@MichMich MichMich merged commit c0ddc02 into MagicMirrorOrg:develop Jan 24, 2021
@MichMich
Copy link
Collaborator

Thanks for your great contribution!

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