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

Added Time#middle_of_day method #10879

Merged
merged 1 commit into from Jul 29, 2013
Merged

Added Time#middle_of_day method #10879

merged 1 commit into from Jul 29, 2013

Conversation

@makaroni4
Copy link
Contributor

@makaroni4 makaroni4 commented Jun 7, 2013

No description provided.

@sikachu
Copy link
Member

@sikachu sikachu commented Jun 7, 2013

I looked at #10878 and still doesn't see a rationale behind this. Would you mind telling us why this is useful to add?

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jun 7, 2013

@sikachu sure

When one need to make a query relative to middle of the day (for example to show weather data, I faced this situation today) this method might be useful. Also there is Time#midnight method, but no Time#midday.

For sure initializer is a good place for keeping extensions like this, it is not a problem.

@senny
Copy link
Member

@senny senny commented Jun 14, 2013

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jun 14, 2013

@makaroni4 needs adding to DateTime as well

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jun 14, 2013

Thinking about it, adding morning? and afternoon? would be useful too.

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jun 14, 2013

@pixeltrix thx, I will add midday to DateTime 👍

midday is the opposite of midnight, which difened for Time and DateTime so it is one of the reasons for me to add this method.

There is no opposite method for morning and afternoon (I think it should be smth like midnight) so I think it is a perfect scenario when initializer should be used.

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jun 14, 2013

There is past?, future? and today? though which is why I suggested adding morning? and afternoon?. Also missing are tomorrow? and yesterday?.

@killthekitten
Copy link
Contributor

@killthekitten killthekitten commented Jun 14, 2013

Both hands are up for the tomorrow? and yesterday?

@PavelDemyanenko
Copy link

@PavelDemyanenko PavelDemyanenko commented Jun 14, 2013

This might be usefull

@schneems
Copy link
Member

@schneems schneems commented Jun 18, 2013

Interesting, could potentially find this useful. @makaroni4 are you going to add this to DateTime ?

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jun 18, 2013

Hi, @schneems!

For sure, give me couple of minutes.

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jun 18, 2013

@pixeltrix @schneems done.

I added methods for both Date and DateTime.

I see duplications of methods like beginning_of_day, end_of_day in Time and DateTime. Should we move them to DateAndDateTime module?

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jul 25, 2013

@steveklabnik could you please take a look? This one should be either merged or closed 😃

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Jul 25, 2013

I like to let @pixeltrix merge stuff related to time.

@guilleiguaran
Copy link
Member

@guilleiguaran guilleiguaran commented Jul 25, 2013

@pixeltrix aka Kronos, god of the time 😁

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jul 25, 2013

@makaroni4 I hadn't merged it yet because the build failed and I thought you might be adding the other methods that had been suggested. No matter - can you check out why the tests are failing and update the PR. There is a significant amount of duplication between DateTime and Time calculations but we'll leave that for another PR.

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jul 25, 2013

@pixeltrix I see that it was activerecord test failed: Failed components: activerecord:postgresql:isolated.

Could you please restart the build (I probably have no access, because button is locked on Travis)? I have no such an error on my local machine.

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jul 27, 2013

@pixeltrix the build is green 😄 👍 You rebuilt it or how did it happen?

I will work on DRYing date and time modules in the next PR (as long as tomorrow? and yesterday? methods).

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jul 28, 2013

@makaroni4 can you squash the commits - once you've done that I'll merge 👍

Added middle_of_day method to Date and DateTime
@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jul 29, 2013

@pixeltrix the build is failed again due to bundler error in AR tests: https://travis-ci.org/rails/rails/jobs/9582396

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jul 29, 2013

@makaroni4 rebuilding that job now - we're nearly there 😄

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jul 29, 2013

😄

pixeltrix added a commit that referenced this pull request Jul 29, 2013
Added Time#middle_of_day method
@pixeltrix pixeltrix merged commit c901056 into rails:master Jul 29, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jul 29, 2013

@makaroni4 thanks for you contribution and your patience ❤️

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Jul 29, 2013

Yay! @pixeltrix thank you very much 😃 ❤️

@kirs
Copy link
Member

@kirs kirs commented Sep 9, 2013

BTW, what about changelog?

rafaelfranca added a commit that referenced this pull request Sep 12, 2013
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Sep 12, 2013

Huge 👍 for CHANGELOG. Added

@makaroni4
Copy link
Contributor Author

@makaroni4 makaroni4 commented Sep 12, 2013

@rafaelfranca sorry, I am late (back to school 😃 )

Thanks a lot for the CHANGELOG 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.