-
Notifications
You must be signed in to change notification settings - Fork 121
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
Utilizing current time instead of matching reference timestamp field #100
Comments
Thanks for your feedback, but I don't see what's wrong with the timestamps. The single timestamp ( What kind of improvement do you see? |
That's the behavior I expected, but I don't think that's what it's doing. All of my read_marks have the exact same time as created_at (which I'm using as a reference). Line 137 in 7513518
|
You are right, not the reading time is stored, it's the reference time of the readable. Sorry - it's five year old code :) Using the current time feels better for me. But beware, the GarbageCollector has to be changed/rewritten for this. This line needs the current implementation: Nevertheless, I'm open for a PR. |
Two notes about your "aging report":
|
@ledermann Thank you for the quick response (and the gem). Luckily in this circumstance, I'm neither calling garbage collection nor letting them read multiple items / mark all read. I'll take a look at the gc code and submit a pr in a few. Thanks! |
Hi! I assume that the PR never existed, right? Any tips on what to change in the gem to get the read time instead the created_at timestamp? |
What's the argument for utilizing matching the timestamp of the read_mark with the reference timestamp field? Why not just set the current time?
I was in the middle of building a derivative read report by left joining the data, and was surprised to see the timestamp field matched every single created_at. My intention was to build an "aging" report that showed how long it took to read an object.
Based on your response, if you'd like, I'll submit a PR to set it to current time instead.
The text was updated successfully, but these errors were encountered: