-
Notifications
You must be signed in to change notification settings - Fork 968
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
all.equal.data.table gets ignore.indices argument #6136
base: master
Are you sure you want to change the base?
Conversation
@@ -104,8 +104,8 @@ fsetequal = function(x, y, all=TRUE) { | |||
|
|||
# all.equal ---- | |||
|
|||
all.equal.data.table = function(target, current, trim.levels=TRUE, check.attributes=TRUE, ignore.col.order=FALSE, ignore.row.order=FALSE, tolerance=sqrt(.Machine$double.eps), ...) { | |||
stopifnot(is.logical(trim.levels), is.logical(check.attributes), is.logical(ignore.col.order), is.logical(ignore.row.order), is.numeric(tolerance), is.data.table(target)) | |||
all.equal.data.table = function(target, current, trim.levels=TRUE, check.attributes=TRUE, ignore.indices=FALSE, ignore.col.order=FALSE, ignore.row.order=FALSE, tolerance=sqrt(.Machine$double.eps), ...) { |
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.
Any reason why we are adding a new argument in the middle? This might break downstream code.
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 decided to group ignore.indices with check.attributes as they seem most related, as indices are attributes. I would say that users should always pass all.equal's optional arguments by name, since these are usually passed on recursively, which makes positional argument passing risky with all.equal beyond the first two. If you want me to, I can move ignore.indices to the end.
What about keys? |
From my view the case for ignoring keys is less strong, since they do not appear automatically (I believe?) and also change the behaviour of the data.table object in more relevant ways. Ultimately that would be your decision, I can also implement ignoring keys (with an |
Nah, keys are more intrusive since they also change the layout of how the data is stored, so I share your opinion that they are substantial and it might be less common to ignore them. What just crossed my mind is instead of adding a new argument we could extend |
It would be nicer if it were called In any case, the if (!ignore.indices) { would need to be changed to if (!"index" %in% ignore.attributes) { |
Any opinions on which route you want to go with this? Should |
Indices can now be ignored in
all.equal()
whenignore.indices
is set:closes #6134