-
Notifications
You must be signed in to change notification settings - Fork 235
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
last
option ends at current time instead of current period
#217
Comments
Hey @kapso, it was designed to end at the current time, but I can see how ending at the end of the current period would be more intuitive. It'd be safest to wait until Groupdate 5 to make this change. |
last
option ends at current time instead of current period
@ankane thanks for the response, and your work on this gem. Do you know how far is version 5? |
My best guess would be a year or so. |
@ankane Thats further away. I was thinking maybe this change can be config driven - meaning default will be the current behavior unless someone does this... Groupdate.configure do |config|
config.end_range = :current_period # or :current_time
end ...in which case maybe this doesn't need to wait for version 5? |
I don't think it's worth the effort to add an option. You can always fork if you need it now. I've added it to #218. |
@ankane Got it, I understand. I will probably fork it. It will be helpful if you can point me to the part which is grabbing the current time? Thanks a lot. |
groupdate/lib/groupdate/series_builder.rb Line 120 in f9c824a
|
I am running the following query, which looks fairly standard...
store.orders.group_by_day(:order_at, last: 1).count
...and I am seeing the following data range in the query -
("orders"."order_at" >= '2019-04-02 00:00:00' AND "orders"."order_at" <= '2019-04-02 01:52:42.823299')
...but I feel the end part of date range may not be correct? Shouldn't that be
2019-04-02 23:59:59
instead of2019-04-02 01:52:42.823299
??Using the following versions:
Rails:
5.2.3
Ruby:
2.6.2
groupdate:
4.1.1
This is on my local macOS, with PST as the timezone.
The text was updated successfully, but these errors were encountered: