-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
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
Add forecast service call for extra attributes for nws #117254
Add forecast service call for extra attributes for nws #117254
Conversation
Hey there @kamiyo, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Need to add tests and documentation. @kamiyo Opening now in draft so you also have a chance to review in progress. |
@MatthewFlamm What do you think about limiting the get_detailed_forecast service to only return the detailed_forecast attribute and datetime? |
More specifically actually, what if we used a service like
|
I LOVE this idea. Need to think it through so we have lower chance of breaking change later. My proposal that builds off yours: One service that returns extra attributes for the existing forecast, i.e. don't include the attributes returned by Then a future service could return specified values (or by default all values?) from the |
Also your proposal will greatly simplify this PR as there is a lot of dancing needed with typing to include the already available forecast data in the new service. This is a sign that the current PR will be brittle to changes in the core weather integration code. |
Sounds good! I share your caution about the two namings being too close. One option is since we have it called The other option is we rename the Thoughts? |
There could be other attributes added to this |
Extra and raw makes sense to me! |
Made the changes that should get us close to this proposal. This is again ready for codeowner review. Last TODO before marking generally ready for review is documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Just a thought: should we allow asking for detailed_description for hourly? I don't think the nws api ever returns anything for hourly. But I guess they could choose to do so in the future, though unlikely. Or perhaps we should put a note in the docs about hourly descriptions returning |
Good point. Since NWS returns it, I have no idea if some grid points station returns valid data there. But given that it could will confusion, I suppose we should remove it and add it back if someone asks for it. To keep both types, I could add |
Co-authored-by: G Johansson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the breaking change section in the PR description as the standard forecast return has changed and inform about using the new service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MatthewFlamm 👍
Breaking change
NWS weather entities no longer have a
detailed_description
in the return from theweather.get_forecasts
service. A new service,nws.get_forecasts_extra
, is provided that includesdetailed_description
. Thedetailed_description
is no longer provided forhourly
forecasts in thenws.get_forecasts_extra
service as the API does not return data, however ashort_description
is now available.Proposed change
NWS currently has an unsupported attribute in the weather platform forecast. This PR removes this attribute and introduces a new nws service that returns the forecast with the additional attribute.
This fixes a bug related to weather template validation with the extra attribute.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: