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

Add forecast service call for extra attributes for nws #117254

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented May 11, 2024

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

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @kamiyo, mind taking a look at this pull request as it has been labeled with an integration (nws) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of nws can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign nws Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@MatthewFlamm
Copy link
Contributor Author

Need to add tests and documentation. @kamiyo Opening now in draft so you also have a chance to review in progress.

@kamiyo
Copy link
Contributor

kamiyo commented May 13, 2024

@MatthewFlamm What do you think about limiting the get_detailed_forecast service to only return the detailed_forecast attribute and datetime?

@kamiyo
Copy link
Contributor

kamiyo commented May 13, 2024

More specifically actually, what if we used a service like get_forecast_extra_attributes or get_forecast_attributes, in which you can pass in the name of the attribute you want. So in this case you'd add in the data attribute: detailed_forecast. I'm thinking of this because I eventually want to add in the code the ability to use https://github.com/MatthewFlamm/pynws/blob/f05016196dbf15e1244964d3d905b0a8cfd7053a/src/pynws/forecast.py#L120C9-L120C30 to add the extra forecast attributes into the response given the forecast times. Then you could call the service like so:

service: nws.get_forecast_extra_attributes
target:
  entity_id:
    - weather.kojc
data:
  type: twice_daily
  attributes: ['humidity', 'detailed_forecast', 'probabilityOfThunder', 'mixingHeight', 'pressure']

@MatthewFlamm
Copy link
Contributor Author

More specifically actually, what if we used a service like get_forecast_extra_attributes or get_forecast_attributes, in which you can pass in the name of the attribute you want. So in this case you'd add in the data attribute: detailed_forecast. I'm thinking of this because I eventually want to add in the code the ability to use https://github.com/MatthewFlamm/pynws/blob/f05016196dbf15e1244964d3d905b0a8cfd7053a/src/pynws/forecast.py#L120C9-L120C30 to add the extra forecast attributes into the response given the forecast times. Then you could call the service like so:

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 weather.get_forecasts. We will still include the attributes that give context like time and whether it is daytime (for twice daily). This one should be named consistently with weather.get_forecasts: nws.get_extra_forecasts or nws.get_forecasts_extra?

Then a future service could return specified values (or by default all values?) from the detailed_forecast at specified times (the default is now?). This could be nws.get_detailed_forecasts. However I'm not sure if that will get confused with detailed_description.

@MatthewFlamm
Copy link
Contributor Author

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.

@kamiyo
Copy link
Contributor

kamiyo commented May 13, 2024

Sounds good!

I share your caution about the two namings being too close.

One option is since we have it called detailed_description already, we keep that because probably people are expecting that consistent naming i.e. get_detailed_descriptions. Then we have to name the other thing like get_raw_attributes (the nws API describes that call as "Returns raw numerical forecast data for a 2.5km grid area").

The other option is we rename the detailed_description to textual_description or textual_forecast, and then use get_detailed_forecast for all the rest of the attributes. Which fits more what the data is, but is not historically consistent with the integration.

Thoughts?

@MatthewFlamm
Copy link
Contributor Author

There could be other attributes added to this extras forecast in the future. The naming is hard. What about extra and raw? The naming of raw is somewhat nice in that it will be more low level (and we probably won't do unit conversions?).

@kamiyo
Copy link
Contributor

kamiyo commented May 13, 2024

Extra and raw makes sense to me!

@MatthewFlamm MatthewFlamm changed the title Add detailed forecast service call for nws Add forecast service call for extra attributes for nws May 13, 2024
@MatthewFlamm
Copy link
Contributor Author

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.

@MatthewFlamm
Copy link
Contributor Author

What this looks like in developers tools services

Screenshot_20240513-210421

Screenshot_20240513-210440

Copy link
Contributor

@kamiyo kamiyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@kamiyo
Copy link
Contributor

kamiyo commented May 14, 2024

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 "" and that it's not an error.

@MatthewFlamm
Copy link
Contributor Author

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 dewpoint to both.

@MatthewFlamm
Copy link
Contributor Author

New example returned data for twice_daily below and hourly does not have detailed_description.

Screenshot_20240514-065547

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented May 22, 2024

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 weather.get_forecasts so I propose this alignment in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weather template validation is too strict and/or NWS integration returns bad forecasts
2 participants