Beginning and end of hour support for Time and DateTime #6156

Merged
merged 1 commit into from May 4, 2012

Conversation

Projects
None yet
4 participants
@mjtko
Contributor

mjtko commented May 4, 2012

This feature was initially proposed in #598.

This pull request consists of implementations and test coverage fro #beginning_of_hour and #end_of_hour methods for Time and DateTime core extensions within activesupport. No implementation has been provided for Date (beginning_of_hour and end_of_hour for a Date seems nonsensical).

While this feature has been based on master, I'm more than happy to submit it against 3-2-stable for any future 3.2.x release.

For anybody wanting to monkey patch their existing Rails applications with this code, take this gist and place in an initializer: https://gist.github.com/2595181

@larzconwell

This comment has been minimized.

Show comment Hide comment
@larzconwell

larzconwell May 4, 2012

Contributor

Awesome I like beginning_of_hour. I'd like 3.2 support as well (: 👍

Contributor

larzconwell commented May 4, 2012

Awesome I like beginning_of_hour. I'd like 3.2 support as well (: 👍

@mjtko

This comment has been minimized.

Show comment Hide comment
@mjtko

mjtko May 4, 2012

Contributor

@larzconwell Ok, I've also applied the above commit against 3-2-stable in my feature-beginning_of_hour-for-3-2-stable branch. The commit can be cherry-picked from/viewed etc. here: 145cc69

I can open a second pull request for that branch to 3-2-stable if anybody from the Rails core team wants to instruct me to do so. :-)

Contributor

mjtko commented May 4, 2012

@larzconwell Ok, I've also applied the above commit against 3-2-stable in my feature-beginning_of_hour-for-3-2-stable branch. The commit can be cherry-picked from/viewed etc. here: 145cc69

I can open a second pull request for that branch to 3-2-stable if anybody from the Rails core team wants to instruct me to do so. :-)

@larzconwell

This comment has been minimized.

Show comment Hide comment
@larzconwell

larzconwell May 4, 2012

Contributor

@jeremy What do you think about this?

Contributor

larzconwell commented May 4, 2012

@jeremy What do you think about this?

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 4, 2012

Member

I've wanted beginning_of_hour before. When have you needed end_of_hour, or is it just for symmetry?

Made some implementation comments inline.

Member

jeremy commented May 4, 2012

I've wanted beginning_of_hour before. When have you needed end_of_hour, or is it just for symmetry?

Made some implementation comments inline.

@@ -91,6 +91,17 @@ def end_of_day
change(:hour => 23, :min => 59, :sec => 59)
end
+ # Returns a new DateTime representing the start of the hour (hh:00:00)
+ def beginning_of_hour
+ change(:min => 0)

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 4, 2012

Member

Should this change seconds and usec to 0 also?

@jeremy

jeremy May 4, 2012

Member

Should this change seconds and usec to 0 also?

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 4, 2012

Member

Ah nevermind, setting :min zeroes :sec.

@jeremy

jeremy May 4, 2012

Member

Ah nevermind, setting :min zeroes :sec.

jeremy added a commit that referenced this pull request May 4, 2012

Merge pull request #6156 from mjtko/feature-beginning_of_hour
Beginning and end of hour support for Time and DateTime

@jeremy jeremy merged commit fecfd61 into rails:master May 4, 2012

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy May 4, 2012

Member

Backport welcome to 3-2-stable :)

Member

jeremy commented May 4, 2012

Backport welcome to 3-2-stable :)

@larzconwell

This comment has been minimized.

Show comment Hide comment
@larzconwell

larzconwell May 4, 2012

Contributor

I haven't need end_of_hour but I'm sure it would come in handy at some point, plus it's nice to keep the beginning and ending together. :D

Contributor

larzconwell commented May 4, 2012

I haven't need end_of_hour but I'm sure it would come in handy at some point, plus it's nice to keep the beginning and ending together. :D

@mjtko

This comment has been minimized.

Show comment Hide comment
@mjtko

mjtko May 5, 2012

Contributor

Yup, end_of_hour is really for the symmetry and to conform to the principle of least surprise should somebody find they need it. In general, the uses for end_of_X seem to be less common than uses for beginning_of_X. :)

I'll open a pull request from my 3-2-stable feature branch for the backport.

Contributor

mjtko commented May 5, 2012

Yup, end_of_hour is really for the symmetry and to conform to the principle of least surprise should somebody find they need it. In general, the uses for end_of_X seem to be less common than uses for beginning_of_X. :)

I'll open a pull request from my 3-2-stable feature branch for the backport.

@vijaydev

This comment has been minimized.

Show comment Hide comment
@vijaydev

vijaydev May 5, 2012

Member

@mjtko Hey, can you please document this in the AS core extensions guide?

Member

vijaydev commented May 5, 2012

@mjtko Hey, can you please document this in the AS core extensions guide?

@mjtko

This comment has been minimized.

Show comment Hide comment
@mjtko

mjtko May 5, 2012

Contributor

Sure, no problem, will get on that later today (UK).

Contributor

mjtko commented May 5, 2012

Sure, no problem, will get on that later today (UK).

@mjtko

This comment has been minimized.

Show comment Hide comment
@mjtko

mjtko May 7, 2012

Contributor

Hi @vijaydev,

Please see lifo/docrails@8c4777f446d9e14deb077f44d31579a5a96e68ae . I guess this will want to be brought through to the 3-2-stable banch too given the backport in #6170.

Hope I've done the correct thing in pushing this to docrails; if not, let me know and I'll revert and open pull requests instead.

Cheers.

Contributor

mjtko commented May 7, 2012

Hi @vijaydev,

Please see lifo/docrails@8c4777f446d9e14deb077f44d31579a5a96e68ae . I guess this will want to be brought through to the 3-2-stable banch too given the backport in #6170.

Hope I've done the correct thing in pushing this to docrails; if not, let me know and I'll revert and open pull requests instead.

Cheers.

@mjtko

This comment has been minimized.

Show comment Hide comment
@mjtko

mjtko May 7, 2012

Contributor

Sorry, I just spotted a mistake. Now corrected in lifo/docrails@61587b1

Also, while I'm adding another comment, I also spotted a minor issue with the existing example dates for beginning_of_day and end_of_day. That one is in lifo/docrails@f174e8d

Over to you. :-)

Contributor

mjtko commented May 7, 2012

Sorry, I just spotted a mistake. Now corrected in lifo/docrails@61587b1

Also, while I'm adding another comment, I also spotted a minor issue with the existing example dates for beginning_of_day and end_of_day. That one is in lifo/docrails@f174e8d

Over to you. :-)

@vijaydev

This comment has been minimized.

Show comment Hide comment
@vijaydev

vijaydev May 7, 2012

Member

docrails is fine. I will backport when merging to master. Thanks.

Member

vijaydev commented May 7, 2012

docrails is fine. I will backport when merging to master. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment