Unexpected behaviour for optional arguments.
Assume that a custom class has been created similar to this issue
setClass('myMethod')
setMethod('coerce', c(from = 'ANY', to = 'myMethod'), function(from, to){
if(!from %in% c('hello', 'world'))
stop('this should not happen')
from
})
p <- arg_parser('test-parser')
p <- add_argument(p, arg = '--my-number', help = 'a number', type = 'numeric')
p <- add_argument(p, arg = c('--myMethod'),
help = ('a test variable'),
type = 'myMethod')
Now as both arguments are optional, we’d assume that providing one or more of them, would do just fine. As such
parse_args(p)
works, but in the case where the first argument is provided, this fails
parse_args(p, c('--my-number', '4.2'))
The issue here, is that line line 521-523 within parse_args makes no assumption for partially missing arguments
if (length(arg.idx) > 0) {
# extract values following the optional argument label
x[match(argv[arg.idx], arg.opt)] <- argv[arg.idx+1];
# convert type of extraced values; x is now a list
x <- mapply(convert_type, ##<===== Sinner
object=x, class=arg.opt.types, nargs=arg.opt.nargs, ##<===== Sinner
SIMPLIFY=FALSE); ##<===== Sinner
# remove extracted arguments
to.remove <- c(arg.idx, arg.idx+1);
argv <- argv[-to.remove];
}
Several fixes are possible here. The simplest is likely just to change mapply(convert_type, ...
to
mapply(function(object, class, nargs){
if(is.na(object))
return(object)
else
convert_type(object = object, class = class, nargs = nargs)
}, ...
or similarly updating the definition of convert_type
.
A more complex (but faster) solution would be to continue the subsetting
if (length(arg.idx) > 0) {
# extract values following the optional argument label
x[ind <- match(argv[arg.idx], arg.opt)] <- argv[arg.idx+1];
# convert type of extraced values; x is now a list
x[ind] <- mapply(convert_type,
object = x[ind],
class = arg.opt.types[ind],
nargs = arg.opt.nargs[ind],
SIMPLIFY = FALSE);
# remove extracted arguments
to.remove <- c(arg.idx, arg.idx+1);
argv <- argv[-to.remove];
}
Comments (6)
-
repo owner -
reporter Thank you David. This behaviour seems odd either way, and in the end, did not have anything to do with the user-defined type, but more that the function evaluates all optional arguments given any optional arguments. Although, I still think it is worth considering User-defined arguments. Often it is useful to be able to restrict numeric arguments to positive only values, certain intervals or in the case of files restrict to certain file extensions. Of course one could just check the arguments after they have been parsed, but then one looses the generality of the parser. Your package does seem to be the most well developed on cran from my perspective though.
-
I agree with Oliver. I have this same issue with just standard numeric type. You can reproduce with
library(argparser) p <- arg_parser("Test") p <- add_argument(p, "--arg1", help = "arg1", type = "integer") p <- add_argument(p, "--arg2", help = "arg2", type = "integer") # Get the same results if I put both arguments in one command # p <- add_argument(p, c("--arg1", "--arg2"), # help = c("arg1", "arg2"), # type = c("integer", "integer")) # Works args <- parse_args(p, c("--arg1", "1", "--arg2", "2")) print(args) # Works args <- parse_args(p) print(args) # Fails args <- parse_args(p, c("--arg1", "1"))
The last line produces the error message
Error in (function (object, class, nargs) :Invalid argument value: expecting integer but got: (NA).
I feel like it should be possible to include only a subset of of optional arguments and that should work.
It is a really useful packages anyway.
Here is my session info, but I get the same in other machines and with R 3.6.1 as well
> sessionInfo() R version 4.0.3 (2020-10-10) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 20.04.1 LTS Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0 LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0 locale: [1] LC_CTYPE=es_US.UTF-8 LC_NUMERIC=C LC_TIME=es_MX.UTF-8 LC_COLLATE=es_US.UTF-8 LC_MONETARY=es_MX.UTF-8 [6] LC_MESSAGES=es_US.UTF-8 LC_PAPER=es_MX.UTF-8 LC_NAME=C LC_ADDRESS=C LC_TELEPHONE=C [11] LC_MEASUREMENT=es_MX.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] argparser_0.6 loaded via a namespace (and not attached): [1] colorspace_1.4-1 scales_1.1.1 compiler_4.0.3 plyr_1.8.6 R6_2.4.1 tools_4.0.3 Rcpp_1.0.5 AMOR_0.2-1 [9] lifecycle_0.2.0 munsell_0.5.0 rlang_0.4.8
-
reporter I am expecting that the problem currently is, that the package is only maintained to be functional rather than updated. I have considered extending the package and would love to hear from @David Shih whether this is the actual case?
-
repo owner The two issues described here were actually caused by a bug that mishandled
NA
in theconvert_type
function. This bug was introduced during a feature to allow checking of integer and numeric types.This bug was already fixed in d75d49c6111fa13a15ecd5e6c470b9e4ae6acad6, but the CRAN version lagged behind.
Test cases are added in 31edb6dc10cc07405c2c7bdf85d15ffe75159da1
-
repo owner - changed status to resolved
- Log in to comment
Thanks for your detailed bug report. You might be venturing outside the intended use of this function, but I’ll look into it.