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 missing dew point and humidity in tomorrowio forecasts #99793

Merged

Conversation

lymanepp
Copy link
Contributor

@lymanepp lymanepp commented Sep 7, 2023

Breaking change

Proposed change

Humidity and dew point were added to the tomorrow.io forecasts in 2023.9.0 but they aren't working as two lines were missed in the original commits.

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 Black (black --fast 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

home-assistant bot commented Sep 7, 2023

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

Code owner commands

Code owners of tomorrowio 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 tomorrowio Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@raman325
Copy link
Contributor

raman325 commented Sep 7, 2023

should we add tests for these attributes? I think we check all the other ones in the tests

@raman325 raman325 added this to the 2023.9.1 milestone Sep 7, 2023
@lymanepp lymanepp marked this pull request as ready for review September 7, 2023 03:38
@lymanepp lymanepp requested a review from raman325 as a code owner September 7, 2023 03:38
Copy link
Contributor

@raman325 raman325 left a comment

Choose a reason for hiding this comment

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

We should add tests for these attributes -> we should do this in general to minimize the chances of this happening in the future

@home-assistant
Copy link

home-assistant bot commented Sep 7, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft September 7, 2023 04:23
@lymanepp
Copy link
Contributor Author

lymanepp commented Sep 7, 2023

We should add tests for these attributes -> we should do this in general to minimize the chances of this happening in the future

Tests were added with the original commit. Unfortunately, the existing tests ignore the logic to construct the tomorrow.io API request and always return the contents of v4.json. This is not a new problem or related to these changes.

@lymanepp lymanepp marked this pull request as ready for review September 7, 2023 20:08
@home-assistant home-assistant bot requested a review from raman325 September 7, 2023 20:08
@raman325
Copy link
Contributor

raman325 commented Sep 7, 2023

Tests were added with the original commit. Unfortunately, the tests ignore the logic to construct the tomorrow.io API request and always return the contents of v4.json.

Gotcha, should we then add a test that captures the API request and validates the fields in the payload? I took a brief look at the available mocks and I'm actually not sure if this is possible but I could be wrong.

@lymanepp
Copy link
Contributor Author

lymanepp commented Sep 7, 2023

We should be able to make assertions on mock_update for TomorrowioV4.realtime_and_all_forecasts. But I'm not sure how we'd reference that mock given that it's added automagically.

Edit: disregard previous. I added an assertion to ensure that we're sending the proper args to realtime_and_all_forecasts.

Comment on lines +204 to +205
"dewPoint",
"humidity",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, dewPoint and humidity aren't very useful for forecasts other than hourly intervals (or less) since HA doesn't have min/max values for them and, even if it did, they still wouldn't be very useful since they won't necessarily coincide with the daily high/low temperatures. But to avoid extra API calls, this is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha, just another reason why this API can be so frustrating to use!

@lymanepp lymanepp force-pushed the tomorrowio-missing-humidity-dew-point branch from 41b119e to b5a9062 Compare September 8, 2023 01:06
Copy link
Contributor

@raman325 raman325 left a comment

Choose a reason for hiding this comment

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

thanks for adding the assertion, good way to ensure we don't run into an issue like this again!

@raman325 raman325 merged commit b76ba00 into home-assistant:dev Sep 8, 2023
@lymanepp lymanepp deleted the tomorrowio-missing-humidity-dew-point branch September 8, 2023 02:12
bramkragten pushed a commit that referenced this pull request Sep 8, 2023
* Fix missing dew point and humidity in tomorrowio forecasts

* Add assertion for correct parameters to realtime_and_all_forecasts method
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tomorrow.io integration forecasts are missing humidity and dew point
3 participants