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

drop {assertthat} dependency #243

Merged
merged 1 commit into from
Feb 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .ci/setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ sudo apt-get install \
tidy \
qpdf

Rscript -e "install.packages(c('assertthat', 'covr', 'data.table', 'futile.logger', 'httr', 'jsonlite', 'knitr', 'lintr', 'markdown', 'purrr', 'stringr', 'testthat'), repos = 'https://cran.r-project.org', Ncpus = parallel::detectCores())"
Rscript -e "install.packages(c('covr', 'data.table', 'futile.logger', 'httr', 'jsonlite', 'knitr', 'lintr', 'markdown', 'purrr', 'stringr', 'testthat'), repos = 'https://cran.r-project.org', Ncpus = parallel::detectCores())"
cp test-data/* r-pkg/inst/testdata/
1 change: 0 additions & 1 deletion r-pkg/DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ Description:
Depends:
R (>= 3.3.0)
Imports:
assertthat (>= 0.2.0),
data.table,
futile.logger,
httr,
Expand Down
6 changes: 0 additions & 6 deletions r-pkg/NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@ export(es_search)
export(get_fields)
export(parse_date_time)
export(unpack_nested_data)
importFrom(assertthat,is.count)
importFrom(assertthat,is.flag)
importFrom(assertthat,is.number)
importFrom(assertthat,is.string)
importFrom(assertthat,is.writeable)
importFrom(assertthat,see_if)
importFrom(data.table,":=")
importFrom(data.table,as.data.table)
importFrom(data.table,copy)
Expand Down
77 changes: 60 additions & 17 deletions r-pkg/R/assertions.R
Original file line number Diff line number Diff line change
@@ -1,21 +1,64 @@

# [title] assert_that wrapper
# [name] assert
# [description] When making an assertion you might call:
#
# \code{assertthat::assert_that(assertthat::is.date(x))}
#
# or something like that. This is an alias to \code{\link[assertthat]{assert_that}} to be used
# for two benefits: \enumerate{
# \item{This uses \code{\link{log_fatal}} instead of \code{\link{stop}} on failure}
# \item{Much less clutter in the source code}
# }
#' @importFrom assertthat see_if
.assert <- function(..., msg = NULL) {
res <- assertthat::see_if(..., env = parent.frame(), msg = msg)
if (res) {
# [title] assert something and raise an exception if it isn't true
# [name] .assert
# [description] If the condition passed to .assert() does not evaluate to TRUE,
# issues a FATAL-level log message and then raises an R exception,
# both with the content of `msg`.
.assert <- function(expr, msg) {
res <- eval(expr, env = parent.frame())
if (isTRUE(res)) {
return(invisible(TRUE))
} else {
log_fatal(attr(res, "msg"))
}
log_fatal(msg)
}

# [title] check if an object is a count
# [name] .is_count
# [description] Returns TRUE if `x` is a single positive integer
# and FALSE otherwise.
.is_count <- function(x) {
return(
length(x) == 1 &&
is.numeric(x) &&
!is.na(x) &&
x > 0 &&
trunc(x) == x
)
}

# [title] check if an object is a scalar logical
# [name] .is_flag
# [description] Returns TRUE if `x` is `TRUE` or `FALSE`
# and `FALSE` otherwise.
.is_flag <- function(x) {
return(
is.logical(x) &&
length(x) == 1L &&
!is.na(x)
)
}

# [title] check if an object is a string
# [name] .is_string
# [description] Returns TRUE if `x` is a non-empty string
# and FALSE otherwise.
.is_string <- function(x) {
return(
is.character(x) &&
length(x) == 1L &&
!is.na(x) &&
x != ""
)
}

# [title] check if an object is a writeable filepath that exists
# [name] .is_writeable
# [description] Returns TRUE if `x` is a filepath that already exists
# and is writeable, and FALSE otherwise.
.is_writeable <- function(x) {
return(
.is_string(x) &&
file.exists(x) &&
file.access(x, mode = 2L)[[1L]] == 0L
)
}
4 changes: 2 additions & 2 deletions r-pkg/R/chomp_aggs.R
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ chomp_aggs <- function(aggs_json = NULL) {

# Get first agg name
aggNames <- names(jsonList[["aggregations"]])
assertthat::assert_that(
assertthat::is.string(aggNames)
.assert(
.is_string(aggNames)
, msg = "aggregations are expected to have a single user-assigned name. This is a malformed aggregations response."
)

Expand Down
35 changes: 12 additions & 23 deletions r-pkg/R/es_search.R
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
#' want to change this behavior, provide a path here. `es_search` will create
#' and write to a temporary directory under whatever path you provide.
#' @inheritParams doc_shared
#' @importFrom assertthat is.count is.flag is.number is.string is.writeable
#' @importFrom parallel detectCores
#' @export
#' @examples
Expand Down Expand Up @@ -88,7 +87,7 @@ es_search <- function(es_host
) {

# Check if this is an aggs or straight-up search query
if (!assertthat::is.string(query_body)) {
if (!.is_string(query_body)) {
msg <- sprintf(paste0("query_body should be a single string. ",
"You gave an object of length %s")
, length(query_body))
Expand All @@ -105,32 +104,22 @@ es_search <- function(es_host
}

# assign 1 core by default, if the number of cores is NA
if (is.na(n_cores) || !assertthat::is.count(n_cores)) {
if (is.na(n_cores) || !.is_count(n_cores)) {
msg <- "detectCores() returned NA. Assigning number of cores to be 1."
log_warn(msg)
n_cores <- 1
}

# Other input checks we don't have explicit error messages for
.assert(
assertthat::is.string(es_host)
, es_host != ""
, assertthat::is.string(es_index)
, es_index != ""
, assertthat::is.string(query_body)
, query_body != ""
, assertthat::is.string(scroll)
, scroll != ""
, max_hits >= 0
, assertthat::is.count(n_cores)
, n_cores >= 1
, assertthat::is.flag(break_on_duplicates)
, !is.na(break_on_duplicates)
, assertthat::is.flag(ignore_scroll_restriction)
, !is.na(ignore_scroll_restriction)
, assertthat::is.string(intermediates_dir)
, assertthat::is.writeable(intermediates_dir)
)
# other input checks with simple error messages
.assert(.is_string(es_host), "Argument 'es_host' must be a non-empty string")
.assert(.is_string(es_index), "Argument 'es_index' must be a non-empty string")
.assert(.is_string(query_body), "Argument 'query_body' must be a non-empty string")
.assert(.is_string(scroll), "Argument 'scroll' must be a non-empty string")
.assert(.is_count(max_hits), "Argument 'max_hits' must be a single positive integer")
.assert(.is_count(n_cores), "Argument 'n_cores' must be a single positive integer")
.assert(.is_flag(break_on_duplicates), "Argument 'break_on_duplicates' must be TRUE or FALSE")
.assert(.is_flag(ignore_scroll_restriction), "Argument 'ignore_scroll_restriction' must be TRUE or FALSE")
.assert(.is_writeable(intermediates_dir), "Argument 'intermediates_dir' must be a writeable filepath")

# Aggregation Request
if (grepl("aggs", query_body, fixed = TRUE)) {
Expand Down
5 changes: 3 additions & 2 deletions r-pkg/R/get_fields.R
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ get_fields <- function(es_host
# Input checking
es_url <- .ValidateAndFormatHost(es_host)

# other input checks with simple error messages
.assert(
is.character("es_indices")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

look look, this one was a bug!!!

It was always checking the literal string "es_indices" instead of checking the content of the argument es_indices 😅

, length(es_indices) > 0
is.character(es_indices) && length(es_indices) > 0
, "Argument 'es_indices' must be a non-empty character vector"
)

# collapse character vectors into comma separated strings. If any arguments
Expand Down
6 changes: 1 addition & 5 deletions r-pkg/R/parse_date_time.R
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#'
#' This is a side-effect-free function: it returns a new data.table and the
#' input data.table is unmodified.
#' @importFrom assertthat is.string
#' @importFrom data.table copy is.data.table
#' @importFrom purrr map2 simplify
#' @importFrom stringr str_extract
Expand Down Expand Up @@ -66,10 +65,7 @@ parse_date_time <- function(input_df
}

# Other input checks we don't have explicit error messages for
.assert(
assertthat::is.string(assume_tz)
, assume_tz != ""
)
.assert(.is_string(assume_tz), "Argument 'assume_tz' must be a non-empty string")

# Work on a copy of the DT to avoid side effects
outDT <- data.table::copy(input_df)
Expand Down
2 changes: 1 addition & 1 deletion r-pkg/R/unpack_nested_data.R
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ unpack_nested_data <- function(chomped_df, col_to_unpack) {
msg <- "For unpack_nested_data, chomped_df must be a data.table"
log_fatal(msg)
}
if (!assertthat::is.string(col_to_unpack)) {
if (!.is_string(col_to_unpack)) {
msg <- "For unpack_nested_data, col_to_unpack must be a character of length 1"
log_fatal(msg)
}
Expand Down
67 changes: 67 additions & 0 deletions r-pkg/tests/testthat/test-assertions.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
test_that(".is_count() works", {
expect_true(.is_count(1L))
expect_true(.is_count(8L))
expect_false(.is_count(-2L))
expect_false(.is_count(0))
expect_false(.is_count(15.2))
expect_false(.is_count(NA))
expect_false(.is_count(NA_character_))
expect_false(.is_count(NA_integer_))
expect_false(.is_count(NA_real_))
expect_false(.is_count(c(1L, 2L)))
expect_false(.is_count("a-number"))
expect_false(.is_count(NULL))
expect_false(.is_count(TRUE))
})

test_that(".is_flag() works", {
expect_true(.is_flag(TRUE))
expect_true(.is_flag(FALSE))
expect_false(.is_flag(-1))
expect_false(.is_flag(-1L))
expect_false(.is_flag(0))
expect_false(.is_flag(0L))
expect_false(.is_flag(1))
expect_false(.is_flag(1L))
expect_false(.is_flag(15.2))
expect_false(.is_flag(NA))
expect_false(.is_flag(NA_character_))
expect_false(.is_flag(NA_integer_))
expect_false(.is_flag(NA_real_))
expect_false(.is_flag(c(1L, 2L)))
expect_false(.is_flag("a-number"))
expect_false(.is_flag(NULL))
})

test_that(".is_string() works", {
expect_true(.is_string("abc"))
expect_true(.is_string(" "))
expect_false(.is_string(""))
expect_false(.is_string(-2L))
expect_false(.is_string(0))
expect_false(.is_string(15.2))
expect_false(.is_string(NA))
expect_false(.is_string(NA_character_))
expect_false(.is_string(NA_integer_))
expect_false(.is_string(NA_real_))
expect_false(.is_string(c(1L, 2L)))
expect_false(.is_string(NULL))
expect_false(.is_string(TRUE))
})

test_that(".is_writeable() works", {
expect_true(.is_writeable(getwd()))
expect_false(.is_writeable(file.path(tempdir(), "some-nonsense")))
expect_false(.is_writeable(""))
expect_false(.is_writeable(-2L))
expect_false(.is_writeable(0))
expect_false(.is_writeable(15.2))
expect_false(.is_writeable(NA))
expect_false(.is_writeable(NA_character_))
expect_false(.is_writeable(NA_integer_))
expect_false(.is_writeable(NA_real_))
expect_false(.is_writeable(c(1L, 2L)))
expect_false(.is_writeable("a-number"))
expect_false(.is_writeable(NULL))
expect_false(.is_writeable(TRUE))
})
2 changes: 1 addition & 1 deletion r-pkg/tests/testthat/test-get_fields.R
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ futile.logger::flog.threshold(0)
test_that("get_fields should give an informative error if es_indices is NULL or an empty string", {
expect_error(get_fields(es_host = "http://es.custdb.mycompany.com:9200"
, es_indices = NULL),
regexp = "not greater than 0")
regexp = "Argument 'es_indices' must be a non-empty character vector")
expect_error(get_fields(es_host = "http://es.custdb.mycompany.com:9200"
, es_indices = ""),
regexp = "get_fields must be passed a valid es_indices")
Expand Down
2 changes: 1 addition & 1 deletion r-pkg/tests/testthat/test-integration.R
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ futile.logger::flog.threshold(0)
ver <- uptasticsearch:::.get_es_version(es_host = "http://127.0.0.1:9200")

# is a string
expect_true(assertthat::is.string(ver), info = paste0("returned version: ", ver))
expect_true(.is_string(ver), info = paste0("returned version: ", ver))

# Decided to check that it's coercible to an integer instead of
# hard-coding known Elasticsearch versions so this test won't require
Expand Down