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 yr weather direction #3020

Merged

Conversation

MagMar94
Copy link
Contributor

Fixes so the Yr weather direction is not inverted. This error was reported in the community.

I misunderstood when developing the module because of the wind direction arrow was pointing opposite to the arrow displayed on Yr.no. I renamed the variable to help other developers in the future.

Magnus Marthinsen added 2 commits January 21, 2023 15:55
The weather object expects the wind from direction, not the wind to direction.
This will make it more intuitive for other developers to understand what direction they should provide to the object.
@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2023

Codecov Report

Merging #3020 (6605d68) into develop (cd4ba42) will increase coverage by 0.07%.
The diff coverage is 2.70%.

@@             Coverage Diff             @@
##           develop    #3020      +/-   ##
===========================================
+ Coverage    21.76%   21.84%   +0.07%     
===========================================
  Files           52       52              
  Lines        11441    11441              
===========================================
+ Hits          2490     2499       +9     
+ Misses        8951     8942       -9     
Impacted Files Coverage Δ
modules/default/weather/providers/envcanada.js 0.00% <0.00%> (ø)
modules/default/weather/providers/openmeteo.js 0.00% <0.00%> (ø)
...odules/default/weather/providers/openweathermap.js 0.00% <0.00%> (ø)
modules/default/weather/providers/pirateweather.js 0.00% <0.00%> (ø)
modules/default/weather/providers/smhi.js 0.00% <0.00%> (ø)
modules/default/weather/providers/ukmetoffice.js 0.00% <0.00%> (ø)
...es/default/weather/providers/ukmetofficedatahub.js 0.00% <0.00%> (ø)
modules/default/weather/providers/weatherbit.js 0.00% <0.00%> (ø)
modules/default/weather/providers/weatherflow.js 0.00% <0.00%> (ø)
modules/default/weather/providers/weathergov.js 0.00% <0.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rejas
Copy link
Collaborator

rejas commented Jan 21, 2023

Will have a look at this later when home (and if there are tests for this) Have you checked if every provider handles wind direction the same?

@MagMar94
Copy link
Contributor Author

Have you checked if every provider handles wind direction the same?

No, but I only changed the variable assignment in the Yr-provider.

I renamed the variable for everyone to make It easier to understand what value the weather module expects. If a provider returns the wrong value it will result in wrong cardinal directions.

I missed this when I implemented the provider, because I matched the arrow (showWindDirectionAsArrow) to what was displayed on Yr.no with the mirror. On Yr (and other weather services) arrows points in the direction the wind is blowing, but the current MM-implementation points to where the wind is blowing from. I will fix this in another PR referencing #3019. I created the issue to get some feedback before I fix it.

@rejas rejas merged commit 2e2962d into MagicMirrorOrg:develop Jan 21, 2023
@MagMar94 MagMar94 deleted the bugfix/fix-yr-weather-direction branch January 22, 2023 11:04
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.

3 participants