Fix recorder "year" period in leap year

[peteh]

Proposed change

In leap years the statistic card is omitting December from the data.

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

The calculation for the end data is wrong in leap years when using a statistic card with period set to "year".
The algorithm gets the 01.01. of the year, then adds 365 days and replaces the day with the 1st again.
Thus in a leap year we would add 365 days resulting in the 31st of December in the current year rather than the 1st of January of the next year. In leap years this always results in 01.12.CURRENT_YEAR instead of 01.01.NEXTYEAR.
There might be better solutions to fix it, e.g. calculating current year and then just create an actual period by hard setting day, month and year as a period but I think this should work reliably too.

For 2024

Before the fix:
start: 2024-01-01
end: 2024-12-01

After fix:
start: 2024-01-01
end: 2025-01-01

• This PR fixes or closes issue: fixes Max value does not update in December if set to "this Year" and this year is Leap year #132156
• 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
• 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:

• 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.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

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

[home-assistant]

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (recorder) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of recorder 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 recorder 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.

[emontnemery]

@peteh Can you please give some more details about the fix?

You mention:

In leap years the statistic card is omitting December from the data.

Could you give an example with and without your fix please?

[emontnemery]

Set to draft until this is covered by tests, I'll update the PR with tests shortly

[peteh]

@peteh Can you please give some more details about the fix?

You mention:

In leap years the statistic card is omitting December from the data.

Could you give an example with and without your fix please?

Sure thing,

When you set period to "Year" in a statistic card, it does not take December into the statistic anymore if the year is a leap year.

Broken Setting (Year setting does not take values from this December into account during a leap year - 2024 is a leap year):

Workaround: use fixed_period setting in card

When fixing the bug, both period "year" and fixed period show the same:

It's easier to understand when you walk through this part of the code...

... actually when doing that to answer your question I stumbled on another issue in the same lines of code. One could argue positive offsets are pointless though, as you cannot make statistics for the future, for negative offset the code works well enough for several hundred years as the original comment suggests.

else:  # calendar_period = "year"
            start_time = start_of_day.replace(month=12, day=31) # take current day of year and set the date to the end of the year
            # This works for 100+ years of offset
            start_time = (start_time + timedelta(days=cal_offset * 366)).replace(
                month=1, day=1
            ) #  here actually seems to be another bug - adding 366 will overshoot by 1 in the first year if offset = 1
            end_time = (start_time + timedelta(days=366)).replace(day=1) # here we replaced 365 by 366 as the fix

Looks like I just identified another bug. I didn't want to rewrite it completely because I don't know how to develop properly for homeassistant. I just fixed my original issue and mounted the file into my docker container to test it.

I don't know how to write proper tests for homeassistant to cover the change.
I think a better solution would be to get time of day, set the start date to 01.01 and end date to 31.12. and then just add the offset to the year rather than doing these weird day calculations with adding or subtracting days of the original code.

In hindsight I believe the approach to this problem for all periods in the original code is very error prone and partly incorrect.

[emontnemery]

I think a better solution would be to get time of day, set the start date to 01.01 and end date to 31.12. and then just add the offset to the year rather than doing these weird day calculations with adding or subtracting days of the original code.

The reason the code doesn't do that is that Python stdlib does not support timedeltas specified as weeks, months or years, probably because the meaning of that would not always be clear.

[emontnemery]

Thanks a lot @peteh 👍

[emontnemery]

seems to be another bug - adding 366 will overshoot by 1 in the first year if offset = 1

We should fix that, but it's not crucial because positive periods are not useful for statistics. I'll do it in a follow-up PR.

@peteh Are you active on Discord?

[peteh]

@emontnemery saw your comment a bit late. I updated the PR with a proper fix.

I'm in discord but irregularly. My profile name there should be async or asyncpt if you want to reach out.

[emontnemery]

Your latest commit is not correct, the year now ends at midnight December 31st instead of at midnight January 1st.
Let's revert that commit so we have a working end of year period in today's release, then we have time to make a nicer solution in the future 👍