Repository navigation
Conversation
… columns before or after an anchor column
…inal value. Previously forgot to assign for default case
|
|
||
| setcolorder = function(x, neworder=key(x)) | ||
| # in anchor mode user should provide a length 1 named vector: anchor = c(before/after = anchor_col_name/index) | ||
| setcolorder = function(x, neworder=key(x), anchor = NULL) |
There was a problem hiding this comment.
I prefer the API discussed in the FR with two new arguments. This seems easier to read setcolorder(x, 1L, .before = 2L) than setcolorder(x, 1L, c(before = 2L)).
| # always operating in index mode, consistent with original mode | ||
| oldorder <- seq_along(x) | ||
| if (class(anchor) == "character") { | ||
| anchor_location <- which(names(x) == anchor) |
There was a problem hiding this comment.
Could be simplified to anchor_location = chmatch(anchor, names(x)). Also, the error handling for characters could also be directly in here if(length(anchor_location) == 0L) stop("...")
Also note that the style guide is to use = and not <-.
| if (class(anchor) == "character") { | ||
| anchor_location <- which(names(x) == anchor) | ||
| } else { | ||
| anchor_location <- which(oldorder == anchor) |
There was a problem hiding this comment.
This seems to simplify to anchor_location = as.integer(anchor). Along with it, you could do error handling of if(anchor_location < 1L || anchor_location > length(x)) stop("...") instead of relying on an zero-length integer from which.
| } | ||
| # we always split columns without the moving subset into left, anchor, right, then insert the moving subset before or after anchor. | ||
| # : in R is seq, always return a seqence even for 1:0, or 11:10 of length 10 vector, this is not what we want. | ||
| slice <- function(vec, from, to) { |
There was a problem hiding this comment.
The vec argument does not seem necessary. oldorder = seq_along(x) means that slice will always simplify to if (from > to) return(NULL) else from:to
Related, I do not believe oldorder is needed at all. The only other use, length(oldorder) is the same as length(x)
| } else { | ||
| anchor_location <- which(oldorder == anchor) | ||
| } | ||
| if (length(anchor_location) == 0) { |
There was a problem hiding this comment.
All integers need to have L after them (e.g., if (length(anchor_location) == 0L) {...}). For this example, you can get by with coercing to logical: if (!length(anchor_location)) {...}
| test(498.4, names(setcolorder(DT)), c("b", "a", "c", "d")) | ||
| # Test setcolorder with parameter of anchor | ||
| DT = data.table(a=1,b=2,c=3) | ||
| test(498.5, setcolorder(copy(DT), "c", c(after="a")), data.table(a=1,c=3,b=2)) |
There was a problem hiding this comment.
The builds are erroring based on the test numbering. I believe it will work if you use 498.51, 498.52, 498.53, etc. Alternatively, you can add your tests at the very end with new numbers.
While code coverage can wait as the API is being ironed out, you may want to add a test for a non-integer or character anchor as well as specify the anchor as also being the new order.
setcolorder(copy(DT), 1L, c(before = 1L))
setcolorder(copy(DT), 1L, c(before = list(1L)))
|
@ColeMiller1 Thanks for the review! There are multiple things, I'll first make changes on the more obvious ones. |
|
After reading @ben-schwen 's comments in #4358 I feel I don't need to continue my PR anymore. His code should fully replace mine and is already good to go. |
|
I was overthinking the name of the argument - Regardless, I am closing as you are deferring to the other implementation. Thanks for the PR. |
2 participants
… columns before or after an anchor column
This is per #4668 and #4358.
anchor = c(before = 1). Another option is adding two parametersbefore = NULL, after = NULL. If more people prefer the 2nd interface I can switch to that.beforeorafter, value as numeric, integer or character. There are some checks if any of this failed.If this got accepted, I can further draft the updated help for the function.