Skip to content

Commit

Permalink
fix: Throw an error for unknown short flags and long flags when ``pos…
Browse files Browse the repository at this point in the history 
…itional_arguments=TRUE``

* Throws an error for unknown short flags and long flags when ``positional_arguments=TRUE`` (#34).
  Thanks Greg Minshall for bug report.

Closes #35.
  • Loading branch information
@trevorld
trevorld committed Mar 28, 2020
1 parent 7169d8d commit b5a28d9
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 51 deletions.
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Encoding: UTF-8
Package: optparse
Type: Package
Title: Command Line Option Parser
Version: 1.6.5-2
Version: 1.6.5-3
Authors@R: c(person("Trevor L", "Davis", role = c("aut", "cre"), email="trevor.l.davis@gmail.com"),
person("Allen", "Day", role="ctb", comment="Some documentation and examples ported from the getopt package."),
person("Python Software Foundation", role="ctb", comment="Some documentation from the optparse Python module."),
Expand Down
2 changes: 2 additions & 0 deletions NEWS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ optparse 1.6.5
the default of ``action`` is now ``"callback"``. Thanks Greg Minshall for suggestion (#35).
* Better documentation of ``action=="callback"`` in the man page for ``add_option`` and ``make_option``.
Thanks Greg Minshall for bug report (#35).
* Throws an error for unknown short flags and long flags when ``positional_arguments=TRUE`` (#34).
Thanks Greg Minshall for bug report (#35).

optparse 1.6.4
==============
Expand Down
26 changes: 3 additions & 23 deletions R/optparse.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,14 +569,11 @@ parse_options <- function(object, args, convert_hyphens_to_underscores) {
options_list[[option@dest]] <- option_value
}
if (option@action == "callback") {

callback_fn <- function(...) {
option@callback(option, option@long_flag, option_value, object, ...) # nolint
}
options_list[[option@dest]] <- do.call(callback_fn, option@callback_args)

}

} else {
if (!is.null(option@default) & is.null(options_list[[option@dest]])) {
options_list[[option@dest]] <- option@default
Expand All @@ -600,12 +597,9 @@ parse_args2 <- function(object, args = commandArgs(trailingOnly = TRUE),
# Tells me whether a string is a valid option
.is_option_string <- function(argument, object) {
if (.is_long_flag(argument)) {
if (grepl("=", argument)) {
argument <- sub("(.*?)=.*", "\\1", argument)
}
return(argument %in% .get_long_options(object))
return(TRUE)
} else if (.is_short_flag(argument)) {
return(all(.expand_short_option(argument) %in% .get_short_options(object)))
return(TRUE)
} else {
return(FALSE)
}
Expand All @@ -621,6 +615,7 @@ parse_args2 <- function(object, args = commandArgs(trailingOnly = TRUE),
if (option@long_flag == argument)
return(.option_needs_argument(option))
}
stop("Don't know long flag argument ", argument)
}
} else { # is a short flag
last_flag <- tail(.expand_short_option(argument), 1)
Expand All @@ -634,20 +629,6 @@ parse_args2 <- function(object, args = commandArgs(trailingOnly = TRUE),
# convenience functions that tells if argument is a type of flag and returns all long flag options or short flag options
.is_long_flag <- function(argument) return(grepl("^--", argument))
.is_short_flag <- function(argument) return(grepl("^-[^-]", argument))
.get_long_options <- function(object) {
long_options <- vector("character")
for (ii in seq_along(object@options)) {
long_options <- c(long_options, object@options[[ii]]@long_flag)
}
return(long_options)
}
.get_short_options <- function(object) {
short_options <- vector("character")
for (ii in seq_along(object@options)) {
short_options <- c(short_options, object@options[[ii]]@short_flag)
}
return(short_options)
}
# .expand_short_option based on function by Jim Nikelski (c) 2011
# He gave me a non-exclusive unlimited license to this code
# .expand_short_option("-cde") = c("-c", "-d", "-e") # nolint
Expand All @@ -670,7 +651,6 @@ parse_args2 <- function(object, args = commandArgs(trailingOnly = TRUE),
type <- ifelse(object@type == "NULL", "logical", object@type)
return(c(long_flag, short_flag, argument, type, object@help))
}

.option_needs_argument <- function(option) {
.option_needs_argument_helper(option@action, option@type)
}
Expand Down
45 changes: 18 additions & 27 deletions tests/testthat/test-optparse.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ option_list <- list(
help = "Mean if generator == \"rnorm\" [default \\%default]"),
make_option("--sd", default = 1, metavar = "standard deviation",
help = "Standard deviation if generator == \"rnorm\" [default \\%default]")
)
)
parser_ol <- OptionParser(option_list = option_list)


context("Testing make_option")
Expand Down Expand Up @@ -63,16 +64,16 @@ test_that("parse_args works as expected", {
help = "Print line number at the beginning of each line [default]")
)
parser <- OptionParser(usage = "\\%prog [options] file", option_list = option_list2)
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
expect_equal(sort_list(parse_args(parser_ol,
args = c("--sd=3", "--quietly"))),
sort_list(list(sd = 3, verbose = FALSE, help = FALSE,
count = 5, mean = 0, generator = "rnorm")))
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
expect_equal(sort_list(parse_args(parser_ol,
args = character(0), positional_arguments = TRUE)),
sort_list(list(options = list(sd = 1, help = FALSE, verbose = TRUE,
count = 5, mean = 0, generator = "rnorm"),
args = character(0))))
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
expect_equal(sort_list(parse_args(parser_ol,
args = c("-c", "10"))),
sort_list(list(sd = 1, help = FALSE, verbose = TRUE,
count = 10, mean = 0, generator = "rnorm")))
Expand All @@ -91,15 +92,8 @@ test_that("parse_args works as expected", {
expect_equal(sort_list(parse_args2(parser, args = c("--add-numbers"))),
sort_list(list(options = list(add_numbers = TRUE, help = FALSE),
args = character(0))))
expect_equal(sort_list(parse_args(parser, args = c("-add-numbers", "example.txt"),
positional_arguments = TRUE)),
sort_list(list(options = list(`add-numbers` = FALSE, help = FALSE),
args = c("-add-numbers", "example.txt"))))
expect_error(parse_args(parser, args = c("-add-numbers", "example.txt")))
expect_equal(sort_list(parse_args(parser, args = c("-add-numbers", "example.txt"),
positional_arguments = c(1, 3))),
sort_list(list(options = list(`add-numbers` = FALSE, help = FALSE),
args = c("-add-numbers", "example.txt"))))
expect_error(parse_args(parser, args = c("-add-numbers", "example.txt")), positional_arguments = FALSE)
expect_error(parse_args(parser, args = c("-add-numbers", "example.txt"), positional_arguments = TRUE))
expect_equal(sort_list(parse_args(parser, args = c("--add-numbers", "example.txt"),
positional_arguments = c(1, 3))),
sort_list(list(options = list(`add-numbers` = TRUE, help = FALSE),
Expand Down Expand Up @@ -210,32 +204,29 @@ test_that("test bug when multiple short flag options '-abc' with positional_argu
}
unsorted_list[sort(names(unsorted_list))]
}
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
expect_equal(sort_list(parse_args(parser_ol,
args = c("-qc", "10"), positional_arguments = TRUE)),
sort_list(list(options = list(sd = 1, help = FALSE, verbose = FALSE,
count = 10, mean = 0, generator = "rnorm"),
args = character(0))))
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
args = c("-qcde", "10"), positional_arguments = TRUE)),
sort_list(list(options = list(sd = 1, help = FALSE, verbose = TRUE,
count = 5, mean = 0, generator = "rnorm"),
args = c("-qcde", "10"))))
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
expect_error(parse_args(parser_ol, args = c("-qcde", "10"), positional_arguments = TRUE))
expect_error(parse_args(parser_ol, args = c("a", "b", "c", "d", "e"), positional_arguments = c(1, 3)))
expect_equal(sort_list(parse_args(parser_ol,
args = c("CMD", "-qc", "10", "bumblebee"), positional_arguments = TRUE)),
sort_list(list(options = list(sd = 1, help = FALSE, verbose = FALSE,
count = 10, mean = 0, generator = "rnorm"),
args = c("CMD", "bumblebee"))))
args <- c("CMD", "-qc", "10", "--qcdefg", "--what-what", "bumblebee")
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
args = args, positional_arguments = TRUE)),
sort_list(list(options = list(sd = 1, help = FALSE, verbose = FALSE,
count = 10, mean = 0, generator = "rnorm"),
args = c("CMD", "--qcdefg", "--what-what", "bumblebee"))))
args <- c("CMD", "-qc", "10", "bumblebee", "--qcdefg")
expect_error(parse_args(parser_ol, args = args, positional_arguments = TRUE),
"Don't know long flag argument --qcdefg")
args <- c("-qxc", "10", "bumblebee")
expect_error(parse_args(parser_ol, args = args, positional_arguments = TRUE),
'short flag "x" is invalid')
})

# Bug found by Ino de Brujin and Benjamin Tyner
test_that("test bug when long flag option with '=' with positional_arguments = TRUE", {
expect_equal(sort_list(parse_args(OptionParser(option_list = option_list),
expect_equal(sort_list(parse_args(parser_ol,
args = c("--count=10"), positional_arguments = TRUE)),
sort_list(list(options = list(sd = 1, help = FALSE, verbose = TRUE,
count = 10, mean = 0, generator = "rnorm"),
Expand Down

0 comments on commit b5a28d9

Please sign in to comment.