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

Fix comparison of integers #309

Merged
merged 8 commits into from
Feb 18, 2016
Merged

Fix comparison of integers #309

merged 8 commits into from
Feb 18, 2016

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Oct 11, 2015

Revert to default comparison also if x is integer.

Current behavior:

> testthat::expect_equal(0L, as.Date("2015-10-10Z"))
Error in `-.Date`(x, y) : can only subtract from "Date" objects
9: stop("can only subtract from \"Date\" objects")
8: `-.Date`(x, y)
7: format(x - y, digits = 3)
6: paste0(format(x, digits = 3), " - ", format(y, digits = 3), " == ",
       format(x - y, digits = 3))
5: compare.numeric(actual, expected, ...)
4: compare(actual, expected, ...)
3: condition(object)
2: expect_that(object, equals(expected, label = expected.label,
       ...), info = info, label = label)
1: testthat::expect_equal(0L, as.Date("2015-10-10Z"))

> testthat::expect_equal(0L, "0")
Error in x - y : non-numeric argument to binary operator
7: format(x - y, digits = 3)
6: paste0(format(x, digits = 3), " - ", format(y, digits = 3), " == ",
       format(x - y, digits = 3))
5: compare.numeric(actual, expected, ...)
4: compare(actual, expected, ...)
3: condition(object)
2: expect_that(object, equals(expected, label = expected.label,
       ...), info = info, label = label)
1: testthat::expect_equal(0L, "0")

@@ -145,7 +145,7 @@ compare.numeric <- function(x, y, ..., max_diffs = 10) {

# If they're not the same type or length, fallback to default method
equal <- paste0(equal, collapse = "\n")
if (!is.integer(x) && !is.numeric(y)) return(comparison(FALSE, equal))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a better check would be !identical(class(x), class(y)) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently:

> testthat::expect_equal(1L, 0)
Error: 1L not equal to 0
1 - 0 == 1

I guess this case (and others) will be handled differently if comparing classes of x and y.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's probably reasonable - it doesn't really matter what the values are if the classes are different. But a bigger problem is expect_equal(1, 1L) which for convenience shouldn't fail. Maybe we should special case integer vs. numeric comparison?

This current behaviour is also bad:

expect_equal(1L, factor(1))
#  Error: 1L not equal to factor(1)
# 1 - 1 == NA In addition: Warning message:
# In Ops.factor(x, y) : ‘-’ not meaningful for factors

Copy link
Member

Choose a reason for hiding this comment

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

And this gives an uninformative message:

x <- structure(1L, class = "myclass")
expect_equal(1L, x)
#  Error: 1L not equal to x
# 1 - 1 == 0 

Kirill Müller added 2 commits November 10, 2015 17:15
- if length different, fallback to string comparison
- otherwise, comparison works only for integer, numeric and complex classes
@krlmlr
Copy link
Member Author

krlmlr commented Nov 10, 2015

Updated: Added support for complex, fallback to string comparison if length different, otherwise use numeric comparison only for "true" numbers.

@krlmlr
Copy link
Member Author

krlmlr commented Nov 10, 2015

This probably conflicts with #313.

@krlmlr
Copy link
Member Author

krlmlr commented Feb 18, 2016

Merged with master.

@hadley hadley merged commit f2c98ad into r-lib:master Feb 18, 2016
@krlmlr krlmlr deleted the compare_integer branch February 18, 2016 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants