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: Fix timestamp handling (elapsed; formatting) #111

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

timmc-edx
Copy link
Contributor

  • Use utcnow() instead of now() for elapsed time, because the now() clock can easily change. I was getting "Search found 8 annotations in -17999.895582 seconds." from some runs of pii_check, and I narrowed it down to DjangoSearch(config) changing the clock. Presumably something is hardcoding Eastern Standard Time somewhere. (It's EDT right now!) There might be a better timer utility somewhere, but this should at least be correct when leap seconds and NTP updates aren't involved. :-)
  • Make the timestamp in the report filename go year-month-day rather than year-day-month. Also make it more compact while I'm in there.

Merge checklist:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

- Use `utcnow()` instead of `now()` for elapsed time, because the `now()`
  clock can easily change. I was getting "Search found 8 annotations in
  -17999.895582 seconds." from some runs of pii_check, and I narrowed it
  down to `DjangoSearch(config)` changing the clock. Presumably something
  is hardcoding Eastern Standard Time somewhere. (It's EDT right now!)
  There might be a better timer utility somewhere, but this should at least
  be correct when leap seconds and NTP updates aren't involved. :-)
- Make the timestamp in the report filename go year-month-day rather than
  year-day-month. Also make it more compact while I'm in there.
Copy link
Contributor

@bmtcril bmtcril 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 this! It's been a pain for a long time. It's been lingering for a while, any reason I shouldn't merge and release this change?

@timmc-edx
Copy link
Contributor Author

No reason I'm aware of -- I think I just didn't get around to asking for a review at the time. :-)

@bmtcril bmtcril merged commit 0eb8375 into master Nov 14, 2024
14 checks passed
@bmtcril bmtcril deleted the timmc/utcnow branch November 14, 2024 15:41
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