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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Duration#before and #after as aliases for #ago and #since #27721

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Widdershin
Contributor

Widdershin commented Jan 18, 2017

It's common in test cases at my job to have code like this:

let(:today) { customer_start_date + 2.weeks }
let(:earlier_date) { today - 5.days }

With this change, we can instead write:

let(:today) { 2.weeks.after(customer_start_date) }
let(:earlier_date) { 5.days.before(today) }

I find this to be much more readable, and it makes me happy 馃槃

A small downside I see is that before and after don't make very much sense when used without an argument.

I would be happy to pull these into a new set of methods that always take an argument, but I figured that the lowest impact change was an alias.

I also did not add a test case, as I saw there was no additional test coverage for since and ago. If desired I will happily add a test case.

Please let me know if there's anything else I can address. Additionally, if this change isn't desired, please feel free to close it.

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7
Member

maclover7 commented Jan 18, 2017

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Jan 20, 2017

Member

For comparison, the existing API:

let(:today) { 2.weeks.since(customer_start_date) }
let(:earlier_date) { 5.days.until(today) }
Member

jeremy commented Jan 20, 2017

For comparison, the existing API:

let(:today) { 2.weeks.since(customer_start_date) }
let(:earlier_date) { 5.days.until(today) }
@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 20, 2017

Member

I think the consensus is that existing methods are fine and adding new aliases isn't required.

@Widdershin sorry, but thanks for your suggestion.

Member

pixeltrix commented Jan 20, 2017

I think the consensus is that existing methods are fine and adding new aliases isn't required.

@Widdershin sorry, but thanks for your suggestion.

@pixeltrix pixeltrix closed this Jan 20, 2017

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 20, 2017

Member

I'm quite sympathetic to this. If you gave me the choice between:

let(:today) { 2.weeks.since(customer_start_date) }
let(:earlier_date) { 5.days.until(today) }

And

let(:today) { 2.weeks.after(customer_start_date) }
let(:earlier_date) { 5.days.before(today) }

I'd say the latter requires far less mental gymnastics to read. And I think that's a good example of the strength of alias. It allows us to bend the same instructions to read the most natural in more instances.

Member

dhh commented Jan 20, 2017

I'm quite sympathetic to this. If you gave me the choice between:

let(:today) { 2.weeks.since(customer_start_date) }
let(:earlier_date) { 5.days.until(today) }

And

let(:today) { 2.weeks.after(customer_start_date) }
let(:earlier_date) { 5.days.before(today) }

I'd say the latter requires far less mental gymnastics to read. And I think that's a good example of the strength of alias. It allows us to bend the same instructions to read the most natural in more instances.

@dhh dhh reopened this Jan 20, 2017

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 20, 2017

Member

Actually, I think the after is compelling enough that I'd like to see this make it in. I'd want to write code like the latter.

Member

dhh commented Jan 20, 2017

Actually, I think the after is compelling enough that I'd like to see this make it in. I'd want to write code like the latter.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 20, 2017

Member

Need a CHANGELOG entry explaining its value. Do use the before/after example.

Member

dhh commented Jan 20, 2017

Need a CHANGELOG entry explaining its value. Do use the before/after example.

@Widdershin

This comment has been minimized.

Show comment
Hide comment
@Widdershin

Widdershin Jan 24, 2017

Contributor

@dhh Thanks for your consideration.

I've altered my commit to include a CHANGELOG entry. Let me know if you want anything else added.

Contributor

Widdershin commented Jan 24, 2017

@dhh Thanks for your consideration.

I've altered my commit to include a CHANGELOG entry. Let me know if you want anything else added.

Show outdated Hide outdated activesupport/CHANGELOG.md
@@ -1,3 +1,28 @@
* Add `ActiveSupport::Duration#before` and `#after` as aliases for `#until` and `#since`
It is common in specs to declare dates relative to one another as part of setup.

This comment has been minimized.

@jeremy

jeremy Jan 24, 2017

Member

This is a good example, but it suggests that the API is somehow related to specs or that its value is derived from an edge case usage.

Think a simple before/after with just the code is clear & convincing, e.g.

Add `ActiveSupport::Duration#before` and `#after` as aliases for `#until` and `#since`

These read more like English and require less mental gymnastics to read and write.

Before:

    2.weeks.since(customer_start_date)
    5.days.until(today)

After:

    2.weeks.after(customer_start_date)
    5.days.before(today)
@jeremy

jeremy Jan 24, 2017

Member

This is a good example, but it suggests that the API is somehow related to specs or that its value is derived from an edge case usage.

Think a simple before/after with just the code is clear & convincing, e.g.

Add `ActiveSupport::Duration#before` and `#after` as aliases for `#until` and `#since`

These read more like English and require less mental gymnastics to read and write.

Before:

    2.weeks.since(customer_start_date)
    5.days.until(today)

After:

    2.weeks.after(customer_start_date)
    5.days.before(today)
# Calculates a new Time or Date that is as far in the past
# as this Duration represents.
def ago(time = ::Time.current)
sum(-1, time)
end
alias :until :ago
alias :before :ago

This comment has been minimized.

@jeremy

jeremy Jan 24, 2017

Member

Still feels weird to alias these鈥攁nd leave ago's required argument as optional鈥攔ather than define new methods. Not necessary for this PR, but worth considering

# Calculates this duration in the past relative to +date_or_time+.
def before(date_or_time)
  sum(-1, date_or_time)
end
alias :until :before

def ago
  before ::Time.current
end

That'd break people using the odd 2.days.ago customer_start_date and 5.weeks.since formulations, though, so it'd need deprecation 馃槓

@jeremy

jeremy Jan 24, 2017

Member

Still feels weird to alias these鈥攁nd leave ago's required argument as optional鈥攔ather than define new methods. Not necessary for this PR, but worth considering

# Calculates this duration in the past relative to +date_or_time+.
def before(date_or_time)
  sum(-1, date_or_time)
end
alias :until :before

def ago
  before ::Time.current
end

That'd break people using the odd 2.days.ago customer_start_date and 5.weeks.since formulations, though, so it'd need deprecation 馃槓

This comment has been minimized.

@Widdershin

Widdershin Feb 8, 2017

Contributor

I'm happy to make this change if it's desired, but as you say, perhaps in a different PR. What is the process for deprecating something like that?

@Widdershin

Widdershin Feb 8, 2017

Contributor

I'm happy to make this change if it's desired, but as you say, perhaps in a different PR. What is the process for deprecating something like that?

This comment has been minimized.

@jeremy

jeremy Feb 9, 2017

Member

The process is to allow both old and new method signatures and give a deprecation warning if we see the old one.

# Calculates this duration in the past relative to +date_or_time+.
def before(date_or_time)
  sum(-1, date_or_time)
end
alias :until :before

def ago(deprecated_date_or_time = nil)
  if deprecated_date_or_time
    ActiveSupport::Deprecation.warn "Passing a date_or_time argument to #ago is deprecated and will be removed in Rails 5.2. Use #before(date_or_time) instead."
  end
  before deprecated_date_or_time || ::Time.current
end
@jeremy

jeremy Feb 9, 2017

Member

The process is to allow both old and new method signatures and give a deprecation warning if we see the old one.

# Calculates this duration in the past relative to +date_or_time+.
def before(date_or_time)
  sum(-1, date_or_time)
end
alias :until :before

def ago(deprecated_date_or_time = nil)
  if deprecated_date_or_time
    ActiveSupport::Deprecation.warn "Passing a date_or_time argument to #ago is deprecated and will be removed in Rails 5.2. Use #before(date_or_time) instead."
  end
  before deprecated_date_or_time || ::Time.current
end
@Widdershin

This comment has been minimized.

Show comment
Hide comment
@Widdershin

Widdershin Feb 8, 2017

Contributor

I have resolved the conflicts and updated the CHANGELOG entry as per @jeremy's feedback.

Please let me know if there's anything else you would like done before this is merged.

Contributor

Widdershin commented Feb 8, 2017

I have resolved the conflicts and updated the CHANGELOG entry as per @jeremy's feedback.

Please let me know if there's anything else you would like done before this is merged.

Show outdated Hide outdated activesupport/CHANGELOG.md
@@ -1,3 +1,19 @@
* Add `ActiveSupport::Duration#before` and `#after` as aliases for `#until` and `#since`
These read more like English, and require less mental gymnastics to read and write.

This comment has been minimized.

@jeremy

jeremy Feb 9, 2017

Member

Can chop this comma.

@jeremy

jeremy Feb 9, 2017

Member

Can chop this comma.

@jeremy

Needs test coverage for the new aliases. (If these were removed, no tests would fail, but apps would break.)

@jeremy jeremy added this to the 5.1.0 milestone Feb 9, 2017

@Widdershin

This comment has been minimized.

Show comment
Hide comment
@Widdershin

Widdershin Feb 22, 2017

Contributor

Hi @jeremy, thanks for your review.

I have added test coverage. I included a test for calling before and after with no arguments, following the rationale that since it's part of the public API it should be tested to avoid regressions.

I did not duplicate the more intricate test coverage for since and ago, but I can if desired.

I also removed the comma from my changelog entry.

Please let me know if there are any other changes desired. 馃樃

Contributor

Widdershin commented Feb 22, 2017

Hi @jeremy, thanks for your review.

I have added test coverage. I included a test for calling before and after with no arguments, following the rationale that since it's part of the public API it should be tested to avoid regressions.

I did not duplicate the more intricate test coverage for since and ago, but I can if desired.

I also removed the comma from my changelog entry.

Please let me know if there are any other changes desired. 馃樃

Widdershin added some commits Feb 22, 2017

Add Duration#before and #after as aliases for #ago and #since
It's common in test cases at my job to have code like this:

let(:today) { customer_start_date + 2.weeks }
let(:earlier_date) { today - 5.days }

With this change, we can instead write

let(:today) { 2.weeks.after(customer_start_date) }
let(:earlier_date) { 5.days.before(today) }
@Widdershin

This comment has been minimized.

Show comment
Hide comment
@Widdershin

Widdershin Feb 22, 2017

Contributor

Looks like I have a failure to look into. Strange, as I've adapted that test directly from another one.

Contributor

Widdershin commented Feb 22, 2017

Looks like I have a failure to look into. Strange, as I've adapted that test directly from another one.

@rafaelfranca rafaelfranca removed this from the 5.1.0 milestone Feb 23, 2017

@jeremy jeremy added this to the 5.1.0 milestone Feb 26, 2017

@jeremy jeremy removed the needs feedback label Feb 26, 2017

@jeremy jeremy closed this in 2d84a6b Feb 26, 2017

@Widdershin

This comment has been minimized.

Show comment
Hide comment
@Widdershin

Widdershin Feb 28, 2017

Contributor

Thanks @jeremy for your review and merge, and also for sorting out that test case for me. 馃槃

Contributor

Widdershin commented Feb 28, 2017

Thanks @jeremy for your review and merge, and also for sorting out that test case for me. 馃槃

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