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

Allow DateTime comparison to Infinity/-Infinity. #3457

Closed
wants to merge 1 commit into from

Conversation

joevandyk
Copy link
Contributor

A continuation of the fixes in #544 -- @tenderlove let ActiveRecord support Infinite timestamps, but you can't compare them to other DateTime values. This commit fixes that.

@monde
Copy link
Contributor

monde commented Oct 31, 2011

-1

@joevandyk
Copy link
Contributor Author

damn it mike

@@ -127,6 +127,10 @@ class DateTime

# Layers additional behavior on DateTime#<=> so that Time and ActiveSupport::TimeWithZone instances can be compared with a DateTime
def <=>(other)
super other.to_datetime
if other.respond_to?(:infinite?) and other.infinite? != 0
-other.infinite?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if self is also an infinite you should check for equality and relative signage with other

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DateTime can't be an infinite, I believe. So self could never be an infinite.

@dbenhur
Copy link

dbenhur commented Oct 31, 2011

maybe mike meant -∞ :). I added a comment and test cases to your pull request to cover equality and signage comparisons if both operands are a flavor of infinite.

@joevandyk
Copy link
Contributor Author

Thanks! I'll make those changes soon.

@joevandyk
Copy link
Contributor Author

I added the missing test cases.

@isaacsanders
Copy link
Contributor

Is this still an issue?

@joevandyk
Copy link
Contributor Author

Yes.

@rafaelfranca
Copy link
Member

cc @tenderlove

@ghost ghost assigned tenderlove May 13, 2012
@steveklabnik
Copy link
Member

This will need a big rebase if it's ever to be included.

@steveklabnik
Copy link
Member

@joevandyk ping! any interest in keeping this current?

@joevandyk
Copy link
Contributor Author

this isn't in there yet? :) i'll look at it.

@rafaelfranca
Copy link
Member

Fixed by #7350

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

Successfully merging this pull request may close these issues.

7 participants