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

Weather forecast ignores "maxNumberOfDays" #2018

Closed
mirko3000 opened this issue May 11, 2020 · 7 comments
Closed

Weather forecast ignores "maxNumberOfDays" #2018

mirko3000 opened this issue May 11, 2020 · 7 comments

Comments

@mirko3000
Copy link

Hi there,

currently the config parameter "maxNumberOfDays" of the module "weatherforecast" seems to be ignored - at least it does not work increasing it to more than 7 days. That is because the API needs an additional parameter "cnt", which indicates how many days are to be retrieved. Adding it to the weatherforecast.js in line 305 solves the issue for me:

params += "&cnt=" + this.config.maxNumberOfDays;

Best regards,
Mirko

@MichMich
Copy link
Collaborator

Please send a PR.

Thanks!

@ghost
Copy link

ghost commented May 16, 2020

params += "&cnt=" + (((this.config.maxNumberOfDays < 1) || (this.config.maxNumberOfDays > 128)) ? 40 : this.config.maxNumberOfDays);

@rejas
Copy link
Collaborator

rejas commented May 16, 2020

Maybe add a "Good first bug" to this Issue so people who want to start helping can fix it? Otherwise I will do it sooner than later ;-)

@Ekristoffe
Copy link
Contributor

params += "&cnt=" + (((this.config.maxNumberOfDays < 1) || (this.config.maxNumberOfDays > 128)) ? 40 : this.config.maxNumberOfDays);

I the documentation it said:

How many days of forecast to return. Specified by config.js

Possible values: 1 - 16
Default value: 7 (7 days)
This value is optional. By default the weatherforecast module will return 7 days.

I think only a limit of 16 would be nice.
And i don't think anyone need a forecast of more than 16 days.

@sdetweil
Copy link
Collaborator

And i don't think anyone need a forecast of more than 16 days.

and more importantly, can you GET a forecast from any api for more that 15 days?

@Ekristoffe
Copy link
Contributor

Well I can make it work up to 17 after there is no answer.

Ekristoffe added a commit to Ekristoffe/MagicMirror that referenced this issue May 30, 2020
Weather forecast need the maxNumberOfDays as argument &cnt=**
The minimum is 1 and the maximum is 17.
MichMich added a commit that referenced this issue Jun 2, 2020
@MichMich MichMich closed this as completed Jul 4, 2020
@ember1205
Copy link

I wanted to add a comment here that this issue is not actually "fixed" as the new version now reverts forecasts back to showing only two days (at least when using the Free API). In doing some research, it appears that the actual URL being used to retrieve the forecast is not the correct / current one.

From what I can tell, the base URL is being built out as:

https://api.openweathermap.org/data/2.5/forecast??id={id}&appid={API key}

According to the API docs, it should be built out as:

https://api.openweathermap.org/data/2.5/onecall?lat={lat}&lon={lon}&exclude={part}&appid={API key}

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

No branches or pull requests

6 participants