-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 aliases for prev_year, prev_month, and prev_week in Time and Date calculations #4284
Conversation
👍 Some tests would have been nice. |
What tests would I need to write? I provided some already... |
@mattdbridges I apologize. Perhaps I was too sleepy to see those tests. :) |
The AS guide needs to be updated. cc: @fxn |
@vijaydev How do I go about doing this? or does someone else need to do this? |
@mattdbridges It's expected to be done as part of the patch. You would need to update this file: https://github.com/rails/rails/blob/master/railties/guides/source/active_support_core_extensions.textile. This will reflect in http://edgeguides.rubyonrails.org/active_support_core_extensions.html once the PR is merged. |
@mattdbridges I like the aliases. We have those also in Then, you'd also need to edit the file railties/guides/source/active_support_core_extensions.textile and document the aliases in the corresponding section of each of the three classes somehow. |
@fxn Correct me if I'm wrong, but it looks like I have added these aliases to Date already. These would be in |
Ignore the last comment. I have the |
They have been removed in master because they come with Ruby 1.9. Previous versions defined them for forward compatibility. |
Ah, thanks for the clarification. |
Is this what you were looking for? |
@@ -182,6 +182,13 @@ def prev_week(day = :monday) | |||
result = (self - 7).beginning_of_week + DAYS_INTO_WEEK[day] | |||
self.acts_like?(:time) ? result.change(:hour => 0) : result | |||
end | |||
alias :last_week, :prev_week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why alias
here and alias_method
for the other two? Moreover, this syntax is incorrect for alias. It should be alias :last_week :prev_week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used alias
here as I noticed many other other aliases used it instead of alias_method
. I was unsure of what the convention was, so I went with what I saw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more than willing to change it to whatever you recommend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, but you've to fix the error then. Also in that case, just to be consistent within this PR, please change the two alias_methods below into aliases.
Can you please remove the unrelated merge commits, and squash all your commits into one? |
@vijaydev, Yes, but how do I do that exactly? I am familiar with |
Yes, you've to use interactive rebase In general, do not try to make a topic branch up to date with master by pulling in from master. Just work on your commits, squash and push (with force pushing whenever necessary). |
@vijaydev Thanks for the tip. Actually, I never merged from the master. I am not sure how that unrelated commit ended up in there. Any thoughts on how to get it out of this PR? |
Actually, it's not there anymore. I'll push my squashed commits. |
Added aliases for prev_year, prev_month, and prev_week in Time and Date calculations
As I was working with some time methods, I noticed that using the word
last
made more sense as it is generally how it is thought of. I have added aliases for these three methods last_year, last_month, and last_week.