Forecasts for weather underground#2414
Conversation
|
@pezinek, thanks for your PR! By analyzing the history of the files in this pull request, we identified @arsaboo, @tchellomello and @fabaff to be potential reviewers. |
Landrash
added
new-feature
Landrash
left a comment
Landrash
left a comment
There was a problem hiding this comment.
Looks good with some minor feedback.
|
|
||
| <p class='note'> | ||
| <a name="1d">[1d]</a> - For daily forecasts, you could replace the number | ||
| in ```_1d_``` part of the sensor name only. Valid values are from |
There was a problem hiding this comment.
One set of ` is enough to wrap a variable.
| All the conditions listed above will be updated each 5 minutes with exception of `alerts` that will be updated each 15 minutes by default. | ||
| All the conditions listed above will be updated every 5 minutes. | ||
|
|
||
| <p class='note'> |
There was a problem hiding this comment.
Most of the changes are in notes now. Would it not be preferable to avoid their use for readability?
There was a problem hiding this comment.
Might be easier to follow if the forecast options were broken out into their own section independent of the standard monitoring options.
There was a problem hiding this comment.
OK, I've removed the notes and replaced them with with "plain text".
Dissecting the forecast options to special section would be problematic as they logically belong to the monitored conditions list. But I'm open to further suggestions.
There was a problem hiding this comment.
I think after reading the "additional examples" this will work fine. I would just strongly suggest adding a section header "Forecast" after the words "All the conditions listed above will be updated every 5 minutes." to separate them for readability. Otherwise, I am so anxious to see this merged! Thank you.
There was a problem hiding this comment.
OK, added the "Forecasts" header, how does it look now ?
There was a problem hiding this comment.
Great! Thanks; I think it helped.
|
Updated to address the suggestions from the review. |
Landrash
left a comment
Landrash
left a comment
There was a problem hiding this comment.
One minor feedback but looks good in general.
| </p> | ||
| Monitored conditions marked with <a name="12h">[12h]</a> are 12 hour | ||
| forecasts. To get a forecast for different period/daytime replace the | ||
| `_1d_` part of the sensor name. e.g. `weather_2n` will give you forecast for |
There was a problem hiding this comment.
This one is a mix of 12h and 1d. On purpose of a mistake?
There was a problem hiding this comment.
This is on purpose, in fact what I call 12h forecast here is called daily forecast by the WU API, the problem is that for some conditions they do provide more fine grained granularity and provide values for day and night, and for some they don't, so I just wanted to make the sensor naming a bit more uniform and let users use the same convention, unless they really do care about what happens outside during nighttime.
| All the conditions listed above will be updated each 5 minutes with exception of `alerts` that will be updated each 15 minutes by default. | ||
| All the conditions listed above will be updated every 5 minutes. | ||
|
|
||
| <p class='note'> |
There was a problem hiding this comment.
Great! Thanks; I think it helped.
|
Hi, is there something else expected from me before this PR could be approved ? |
|
I think there is just the one change that Landrash suggested. |
|
Actually I believe I addressed all the issues Landrash pointed out either by fixing the code or by explaining in the comments. Would you mind to point me again to the issues you believe I still have to address ? |
|
The only thing I can see in red is the comment around 115 - "-All the conditions listed above will be updated each 5 minutes with exception of |
|
That one is intentional change and not a mistake, as I explained in the comments, so I believe this PR is ready for approval, unless you strongly disagree with my explanation ;-) |
I've already approved. Just waiting on @Landrash now. |
|
Looks good and can be merged when parent pr is merged. |
|
@Landrash: Thanks ! |
|
@pezinek: And thanks to you for this! I'd almost given up hope of getting this into HA. |
6 participants
Description:
This PR allows forecasts for the Weather Underground component.
Pull request in home-assistant (if applicable): home-assistant/core#7062