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 for dst #56

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Fix for dst #56

merged 3 commits into from
Oct 31, 2023

Conversation

Ptosiek
Copy link
Contributor

@Ptosiek Ptosiek commented Oct 30, 2023

So with DST happening last weekend, found a bug :/

  1. The test were written in a way that they were working only on CET, so I fixed that.
  2. Fix for dst, the offset for end date is taken from this date and not the current one.
  3. I've reworked the code so the filename (and start and end date) are fetched once and passed along the loggers. It is not necessary for the fix. The only downside is that even with fit (only)+cython we open the DB in python once.

fix for dst + fix tests (were working only in CET)
@Ptosiek
Copy link
Contributor Author

Ptosiek commented Oct 31, 2023

There were two issues:

  1. The tests were written to work only if you ran them in a timezone GMT+2 (log at 20:39 UTC, expected file name 22:39). So I guess tests were not working on your side ? (but this was not a big issue)
  2. The local end date epoch time would be incorrectly calculated for historic files/datetimes (bug both in cython and python):
time_t lt_t = time(NULL);
struct tm lt = {0};
localtime_r(&lt_t, &lt);
and using lt.tm_gmtoff
 offset = time.localtime().tm_gmtoff

before this weekend CET was at GMT+2 (now GMT+1), since Sunday the code in test would use 3600s as an offset as it's the current one. But for a date in the past we have to look at what was the tz offset then. Of course it would rarely happen in reality, but the tests were catching it so they were broken. (eg the filename for fit was at H21 instead of H22)

@hishizuka
Copy link
Owner

hishizuka commented Oct 31, 2023

Hi @Ptosiek ,
thank you. I understand.

The previous fix had the correct behavior in my region. The local start time was being output in the file name.
If possible, the time in the file name should be the local start time, not UTC time. It is difficult to tell when the ride is logged. (In Japan, there is a 9-hour time difference, so it is difficult to see at a glance when the ride was logged.)

I'll look at it some more.

@Ptosiek
Copy link
Contributor Author

Ptosiek commented Oct 31, 2023

The time in the filename is still the local one normally. I haven;t changed that: it's just the correct local one :)
Found an issue actually when copying the DB). Will push the fix in a bit

Right the filename was incorrect. Fixed it!

@hishizuka hishizuka merged commit ef0eb6c into hishizuka:master Oct 31, 2023
1 check passed
@hishizuka
Copy link
Owner

Hi @Ptosiek ,
thank you for great code!
datetime.astimezone() is the essence of the modification. Thanks again!

@Ptosiek Ptosiek deleted the fix-dst branch November 2, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants