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

seen: modify/fix stored value & delta calculation for Aware trigger.time #2265

Merged
merged 2 commits into from
May 5, 2022

Conversation

dgw
Copy link
Member

@dgw dgw commented Mar 7, 2022

Description

Fixes a bug on master reported on IRC by monaco (@hedho), related to trigger.time changes from #2099.

I chose to eliminate the bug by using Unix timestamps instead of datetime objects. <datetime_obj>.timestamp() has been around since Python 3.3, so we're safe to use it with 8.x's minimum requirement of Python 3.7.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@dgw dgw added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Mar 7, 2022
@dgw dgw added this to the 8.0.0 milestone Mar 7, 2022
@dgw dgw requested a review from a team March 7, 2022 06:20
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

I think it would be best to add this change (line 68):

- bot.db.set_nick_value(nick, 'seen_timestamp', time.time())
+ bot.db.set_nick_value(nick, 'seen_timestamp', trigger.time.timestamp())

@dgw
Copy link
Member Author

dgw commented Mar 7, 2022

@Exirel That's not related to fixing the bug, though.

Docs for the datetime.timestamp() method specifically say it returns a value "similar to that returned by time.time()". Is there a practical reason for this change other than storing when the line was sent (or received, on nets without server-time) instead of when the seen plugin processes it? The small difference in precision when reporting the time delta later is meaningless at the scale of this plugin's normal use (days or weeks later).

@Exirel
Copy link
Contributor

Exirel commented Mar 7, 2022

Yeah, but it's a good opportunity to be more coherent with the feature. trigger.time is timezone sensitive, and I feel like it makes more sense to use it here as well. Beside, if the event comes from the IRC history (if that ever get implemented), we won't have to update the seen plugin (at least, not that part).

@dgw
Copy link
Member Author

dgw commented Mar 7, 2022

trigger.time is timezone sensitive

Well, this is annoying, but not because of what you said. It's annoying that time.time() isn't always in UTC. Interactive Python session below, which made me quite irritated for a few minutes. 😅

Give me a moment to update the patch.

>>> import time
>>> from datetime import datetime
>>> t = time.time(); dt = datetime.now()
>>> t
1646690791.7546873
>>> dt.timestamp()
1646690791.754754
>>> import pytz
>>> dt.replace(tzinfo=pytz.utc).timestamp()
1646669191.754754

This is for timezone safety.

Co-authored-by: Exirel <florian.strzelecki@gmail.com>
@dgw dgw requested a review from Exirel March 7, 2022 22:02
@dgw dgw changed the title seen: modify calculation to work now that trigger.time is Aware seen: modify/fix stored value & delta calculation for Aware trigger.time Mar 7, 2022
@dgw dgw merged commit b03dca4 into master May 5, 2022
@dgw dgw deleted the seen-aware-datetime branch May 5, 2022 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants