Requesting data from last seven days

[mariwing]

Breaking change

Proposed change

This PR fixes an issue caused by changes in dependency vangorra/python_withings_api@02a3164 that changed default values for date parameters from None to UtcNow. The fix now specify all the optional parameters as None, in effect restoring the old default values.

Thanks to schachar for figuring it out!

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)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes Withings not working in 2021.4.0b3 #48650
• This PR is related to issue:
• Link to documentation pull request:

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
• 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:

• Documentation added/updated for www.home-assistant.io

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.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[homeassistant]

Hi @mariwing,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[probot-home-assistant]

Hey there @vangorra, mind taking a look at this pull request as its been labeled with an integration (withings) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[frenck]

Wait, shouldn't this be reverted/fixed in the upstream library?

[mariwing]

Thank you for the quick review. I don't know where it should be fixed, maybe the new values for the optional parameters makes sense to have in the upstream library? Personally I never liked optional parameters because it could lead to exactly these kind of bugs. Now this code set values instead of relying on "hidden" defaults.

Feel free to decline and close the PR if you think that is the correct thing to do.

[Bluhme1]

But one way or another we need a solution. Could @ronaldheft maybe look at it. It seems that @vangorra has stopped maintaining this Integration

[ronaldheft]

@mariwing The reason the parameters now have a default of a date is due to the arrow 1.0 dependency. The migration guide for arrow 1.0 says this:

arrow.get(None) now returns an error rather than current UTC.

The previous defaults for the parameters were None, hence the reason for one of the errors in a previous version of the library / Home Assistant.

The correct fix is to set to a reasonable start and end date for the date instead of passing None. Passing None on all the parameters will lead to the error that was observed before.

You won't see this error if your Home Assistant install is using an older version of arrow <1.0, which might be what schachar observed in his patch.

Setting a value for the date will fix it for both arrow >= 1.0 and arrow <1.0.

I don't have the time at the moment to test and submit a PR myself, but I would think maybe a time range of 7 days or 30 days may be a reasonable time range? It's only going to return the latest data in the time range specified.

[mariwing]

Thank you @ronaldheft for the explanation! I will do some local testing and update this PR with date values.

[Bluhme1]

@mariwing Good luck. Looking forward to your findings and PR.
Hilsen fra din nabo i Danmark

[mariwing]

@frenck If you have time to review this again that would be great!

After feedback from @ronaldheft I got a better understanding of the issue. I have updated the PR so we now supply dates and ask for data for the last seven days. It has been tested locally and works fine.

My understanding of the underlying code is that it sorts the returned data and uses the latest data point for each type of data. So getting data for the last 7 days means that a measurement needs to have happened during the last 7 days to appear in Home Assistant. I think that is a reasonable balance between fetching large amount of data that will just get discarded and having measurements available on the first run of the integration.

[frenck]

LGTM ✅

Thanks, @mariwing 👍

../Frenck 🚂

[Bluhme1]

Great. Thank you @mariwing @ronaldheft, @schachar and @frenck
Everything works beautifully