Added aliases for prev_year, prev_month, and prev_week in Time and Date calculations #4284

Merged
merged 1 commit into from Feb 28, 2012

Projects

None yet

4 participants

@mdespuits
Contributor

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.

@waseem
Contributor
waseem commented Jan 22, 2012

👍 Some tests would have been nice.

@mdespuits
Contributor

What tests would I need to write? I provided some already...

@waseem
Contributor
waseem commented Jan 23, 2012

@mattdbridges I apologize. Perhaps I was too sleepy to see those tests. :)

@vijaydev
Member

The AS guide needs to be updated. cc: @fxn

@mdespuits
Contributor

@vijaydev How do I go about doing this? or does someone else need to do this?

@vijaydev
Member

@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.

@fxn
Member
fxn commented Feb 14, 2012

@mattdbridges I like the aliases.

We have those also in Date, could you please add them also to Date? That way DateTime will also have them via inheritance.

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 fxn was assigned Feb 14, 2012
@mdespuits
Contributor

Thanks @vijaydev and @fxn. I'll work on these as soon as I can and make push them up.

@mdespuits
Contributor

@fxn Correct me if I'm wrong, but it looks like I have added these aliases to Date already. These would be in active_support/core_ext/date/calculations.rb, am I right?

@mdespuits
Contributor

Ignore the last comment. I have the prev_week method in Date already, but where would the aliases for last_month or last_year go since they are not actually defined in Date?

@fxn
Member
fxn commented Feb 17, 2012

They have been removed in master because they come with Ruby 1.9. Previous versions defined them for forward compatibility.

@mdespuits
Contributor

Ah, thanks for the clarification.

@mdespuits
Contributor

Is this what you were looking for?

@vijaydev vijaydev and 1 other commented on an outdated diff Feb 21, 2012
...port/lib/active_support/core_ext/date/calculations.rb
@@ -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
@vijaydev
vijaydev Feb 21, 2012 Member

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

@mdespuits
mdespuits Feb 21, 2012 Contributor

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.

@mdespuits
mdespuits Feb 21, 2012 Contributor

I am more than willing to change it to whatever you recommend.

@vijaydev
vijaydev Feb 21, 2012 Member

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.

@vijaydev
Member

Can you please remove the unrelated merge commits, and squash all your commits into one?

@mdespuits
Contributor

@vijaydev, Yes, but how do I do that exactly? I am familiar with git rebase -i, but couldn't that potentially harm the references when I push to a remote repo, as in this case?

@vijaydev
Member

Yes, you've to use interactive rebase rebase -i and squash. Since this PR is created from a topic branch, you can do a force push git push -f which will overwrite the commits in this PR with the one you push. Remember to remove the unrelated commits.

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).

@mdespuits
Contributor

@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?

@mdespuits
Contributor

Actually, it's not there anymore. I'll push my squashed commits.

@fxn fxn merged commit 699ba8a into rails:master Feb 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment