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
base: dev
Are you sure you want to change the base?
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 |
This PR does not include enhanced caching for forecast data like was done for #117109 for observation data. This would interact with the caching logic in the built-in |
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: