Skip to content

#187: stop() -> rlang::abort() #192

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.

Sign up for GitHub

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

Open
KoderKow wants to merge 30 commits into
base: main
Choose a base branch
from
Open

#187: stop() -> rlang::abort() #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6a34239
improve error catching for .dittodb_env$db_path
KoderKow Mar 6, 2024
b8c60a5
improve error catching for .dittodb_env$db_path
KoderKow Mar 6, 2024
2517f58
remove checks where they are not needed
KoderKow Mar 6, 2024
b9465ef
add in needed checks
KoderKow Mar 6, 2024
ce29f4b
remove random line
KoderKow Mar 7, 2024
2366ff5
Update capture-requests.R
KoderKow Mar 7, 2024
ff3b46b
export check_db_path so it will be available when needed
KoderKow Mar 7, 2024
fd70417
modularize check db path code
KoderKow Mar 8, 2024
fd3d04c
call 2 env up
KoderKow Mar 8, 2024
8728097
tst
KoderKow Mar 8, 2024
a1726dd
clean up check_db_path
KoderKow Mar 8, 2024
cd65a0a
unit tests for error checking start_db_capturing
KoderKow Mar 8, 2024
8c232d7
updates for testthat 2e
KoderKow Mar 10, 2024
4663040
retrigger checks
KoderKow Mar 10, 2024
65659e9
remove commented out code
KoderKow Mar 27, 2024
c59a081
simplify function
KoderKow Mar 27, 2024
6351072
styling
KoderKow Mar 27, 2024
10180a3
update test with recent function change
KoderKow Mar 27, 2024
e0d5523
update grammar
KoderKow Mar 27, 2024
08926c8
update docs
KoderKow Mar 27, 2024
7bd4323
updates
KoderKow Mar 27, 2024
c54db3f
retrigger checks
KoderKow Apr 6, 2024
c52f0e9
Merge remote-tracking branch 'upstream/main'
KoderKow Apr 8, 2024
ce83bf9
add regex check for all asserts
KoderKow Apr 8, 2024
d68c98b
NEWS update for (#183)
KoderKow Apr 8, 2024
c34866c
first stop -> abort conversion
KoderKow Apr 9, 2024
0532920
convert stop -> abort
KoderKow Apr 9, 2024
3013504
merge with main
KoderKow Apr 9, 2024
8809e87
style
KoderKow Apr 9, 2024
735c347
not sure how i did this
KoderKow Apr 9, 2024
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
11 changes: 5 additions & 6 deletions R/dbQueries-Results.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,12 @@ db_send_query <- function(conn, statement, ...) {

# TDOO: if we are in expect_sql, then we should emit an error (or warning?) with SQL here
if (is_expecting()) {
stop(
"Fixture: ",
make_path(conn@path, get_type(statement), hash(statement)),
"\n",
clean_statement(statement),
call. = FALSE
error_msg <- glue::glue(
"Fixture: {make_path(conn@path, get_type(statement), hash(statement))}
{clean_statement(statement)}"
Comment on lines +43 to +45
Copy link
Collaborator

@jonkeane jonkeane Apr 9, 2024

Choose a reason for hiding this comment

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

What does this look like when it occurs?

I know there are small differences in how newlines are handled (and it might even depend on if cli is installed or not 🙈 ) Would you mind trying to trigger it with and without CLI in your env to see what it looks like and paste that here?

Copy link
Contributor Author

@KoderKow KoderKow Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Reprex:

devtools::load_all()

with_mock_db({
  conn <- dbConnect(RSQLite::SQLite(), dbname = "not_a_db")
})

statement <- "SELECT * FROM dbo.iris"

error_msg <- glue::glue(
  "Fixture: {make_path(conn@path, get_type(statement), hash(statement))}
      {clean_statement(statement)}"
)

error_msg

rlang::abort(error_msg)

Terminal:

image

Question, what do you mean by without CLI? I removed the {cli} package and ran the rlang::abort and it printed the same. I think I am confused :)

My R session 
─ Session info ───────────────────────────────────────────────────────────────────────────────────
 setting  value
 version  R version 4.3.1 (2023-06-16 ucrt)
 os       Windows 10 x64 (build 19045)
 system   x86_64, mingw32
 ui       RStudio
 language (EN)
 collate  English_United States.utf8
 ctype    English_United States.utf8
 tz       America/Chicago
 date     2024-04-10
 rstudio  2023.06.1+524 Mountain Hydrangea (desktop)
 pandoc   NA

─ Packages ───────────────────────────────────────────────────────────────────────────────────────
 !  package     * version date (UTC) lib source
    bit           4.0.5   2022-11-15 [1] CRAN (R 4.3.1)
    bit64         4.0.5   2020-08-30 [1] CRAN (R 4.3.1)
    blob          1.2.4   2023-03-17 [1] CRAN (R 4.3.1)
    brio          1.1.4   2023-12-10 [1] CRAN (R 4.3.3)
    cachem        1.0.8   2023-05-01 [1] CRAN (R 4.3.1)
    cli           3.6.2   2023-12-11 [1] CRAN (R 4.3.3)
    clipr         0.8.0   2022-02-22 [1] CRAN (R 4.3.1)
    DBI         * 1.2.2   2024-02-16 [1] CRAN (R 4.3.3)
    dbplyr        2.4.0   2023-10-26 [1] CRAN (R 4.3.3)
    desc          1.4.3   2023-12-10 [1] CRAN (R 4.3.3)
    details       0.3.0   2022-03-27 [1] CRAN (R 4.3.3)
    devtools      2.4.5   2022-10-11 [1] CRAN (R 4.3.1)
    digest        0.6.34  2024-01-11 [1] CRAN (R 4.3.3)
 VP dittodb     * 0.1.8   2024-03-07 [?] load_all() (on disk 0.1.7.9000)
    dplyr         1.1.4   2023-11-17 [1] CRAN (R 4.3.2)
    ellipsis      0.3.2   2021-04-29 [1] CRAN (R 4.3.1)
    fansi         1.0.6   2023-12-08 [1] CRAN (R 4.3.3)
    fastmap       1.1.1   2023-02-24 [1] CRAN (R 4.3.1)
    fs            1.6.3   2023-07-20 [1] CRAN (R 4.3.1)
    generics      0.1.3   2022-07-05 [1] CRAN (R 4.3.1)
    glue          1.7.0   2024-01-09 [1] CRAN (R 4.3.3)
    grkstyle      0.2.1   2023-10-24 [1] Github (gadenbuie/grkstyle@53051cb)
    hms           1.1.3   2023-03-21 [1] CRAN (R 4.3.1)
    htmltools     0.5.7   2023-11-03 [1] CRAN (R 4.3.3)
    htmlwidgets   1.6.2   2023-03-17 [1] CRAN (R 4.3.1)
    httpuv        1.6.11  2023-05-11 [1] CRAN (R 4.3.1)
    httr          1.4.7   2023-08-15 [1] CRAN (R 4.3.1)
    knitr         1.45    2023-10-30 [1] CRAN (R 4.3.3)
    later         1.3.1   2023-05-02 [1] CRAN (R 4.3.1)
    lifecycle     1.0.4   2023-11-07 [1] CRAN (R 4.3.3)
    magrittr      2.0.3   2022-03-30 [1] CRAN (R 4.3.1)
    memoise       2.0.1   2021-11-26 [1] CRAN (R 4.3.1)
    mime          0.12    2021-09-28 [1] CRAN (R 4.3.0)
    miniUI        0.1.1.1 2018-05-18 [1] CRAN (R 4.3.1)
    pillar        1.9.0   2023-03-22 [1] CRAN (R 4.3.1)
    pkgbuild      1.4.3   2023-12-10 [1] CRAN (R 4.3.3)
    pkgconfig     2.0.3   2019-09-22 [1] CRAN (R 4.3.1)
    pkgload       1.3.4   2024-01-16 [1] CRAN (R 4.3.3)
    png           0.1-8   2022-11-29 [1] CRAN (R 4.3.1)
    profvis       0.3.8   2023-05-02 [1] CRAN (R 4.3.1)
    promises      1.2.1   2023-08-10 [1] CRAN (R 4.3.1)
    purrr         1.0.2   2023-08-10 [1] CRAN (R 4.3.1)
    R6            2.5.1   2021-08-19 [1] CRAN (R 4.3.1)
    Rcpp          1.0.12  2024-01-09 [1] CRAN (R 4.3.3)
    remotes       2.4.2.1 2023-07-18 [1] CRAN (R 4.3.1)
    rlang         1.1.3   2024-01-10 [1] CRAN (R 4.3.3)
    RMariaDB      1.3.1   2023-10-26 [1] CRAN (R 4.3.3)
    RPostgres     1.4.6   2023-10-22 [1] CRAN (R 4.3.3)
    RPostgreSQL   0.7-6   2024-01-11 [1] CRAN (R 4.3.3)
    rprojroot     2.0.4   2023-11-05 [1] CRAN (R 4.3.3)
    RSQLite       2.3.5   2024-01-21 [1] CRAN (R 4.3.3)
    rstudioapi    0.15.0  2023-07-07 [1] CRAN (R 4.3.1)
    sessioninfo   1.2.2   2021-12-06 [1] CRAN (R 4.3.1)
    shiny         1.7.5   2023-08-12 [1] CRAN (R 4.3.1)
    stringi       1.8.3   2023-12-11 [1] CRAN (R 4.3.2)
    stringr       1.5.1   2023-11-14 [1] CRAN (R 4.3.3)
    testthat    * 3.2.1   2023-12-02 [1] CRAN (R 4.3.3)
    tibble        3.2.1   2023-03-20 [1] CRAN (R 4.3.1)
    tidyselect    1.2.0   2022-10-10 [1] CRAN (R 4.3.1)
    urlchecker    1.0.1   2021-11-30 [1] CRAN (R 4.3.1)
    usethis       2.2.2   2023-07-06 [1] CRAN (R 4.3.1)
    utf8          1.2.4   2023-10-22 [1] CRAN (R 4.3.3)
    vctrs         0.6.5   2023-12-01 [1] CRAN (R 4.3.3)
    withr         3.0.0   2024-01-16 [1] CRAN (R 4.3.3)
    xfun          0.42    2024-02-08 [1] CRAN (R 4.3.3)
    xml2          1.3.6   2023-12-04 [1] CRAN (R 4.3.3)
    xtable        1.8-4   2019-04-21 [1] CRAN (R 4.3.1)

 [1] C:/Users/redacted/AppData/Local/R/win-library/4.3
 [2] C:/Program Files/R/R-4.3.1/library

 V ── Loaded and on-disk version mismatch.
 P ── Loaded and on-disk path mismatch.

──────────────────────────────────────────────────────────────────────────────────────────────────

Copy link
Collaborator

@jonkeane jonkeane Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm that's odd. I just tried it locally (thanks for the reprex, that made it super easy!), and the content / spacing is the same, but I do see a (tiny) difference without the cli package:

with cli
image

without cli
image

The difference is very subtle here (just the coloring), but that's what I wanted to confirm. I looked at the docs for rlang::abort and saw that it will use cli to format the messages (see use_cli_format under arguments), and when I clicked through to the page on formatting there was some discussion about changes in format and whitespace — though those might be limited to only when using cli::cli_abort instead of `rlang::abort formatted with cli (I will be honest, I couldn't totally tell from this page when I read it!)

There is another thing on that page that's a little worrying about user input, but that also looks exclusive to cli::cli_abort and not rlang::abort which has been formatted with cli:

> statement <- "SELECT * FROM dbo.iris"
> 
> user_input <- "{base::stop('Wrong message.', call. = FALSE)}"
> 
> error_msg <- glue::glue(
+   "Fixture: ",
+   "{make_path('not_a_db', get_type(statement), hash(statement))}\n",
+   "{clean_statement(statement)}\n",
+   "{user_input}"
+ )
> 
> rlang::abort(error_msg)
Error:
! Fixture: not_a_db/SELECT-eb40ac.R
SELECT * FROM dbo.iris
{base::stop('Wrong message.', call. = FALSE)}
Run `rlang::last_trace()` to see where the error occurred.

So I think this is good — though what do you think about:

    error_msg <-glue(
      "Fixture: {make_path(conn@path, get_type(statement), hash(statement))}\n",
      "{clean_statement(statement)}"
    )

It's mostly a personal preference, but I prefer explicitly escaped new lines as opposed to having it in the quote and overflowing. I also took out the glue:: since it's already imported to dittodb

Copy link
Collaborator

@jonkeane jonkeane Apr 13, 2024

Choose a reason for hiding this comment

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

This also could be incorporated into the abort call, though this is starting to get big enough a separate variable is ok. What do you think?

)

rlang::abort(error_msg)
}

return(new(
Expand Down
3 changes: 2 additions & 1 deletion R/paths.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,5 +64,6 @@ find_file <- function(file_path) {
}
}

stop("Couldn't find the file ", file_path, " in any of the mock directories.")
error_msg <- glue::glue("Couldn't find the file {file_path} in any of the mock directories.")
rlang::abort(error_msg)
Comment on lines +67 to +68
Copy link
Collaborator

@jonkeane jonkeane Apr 13, 2024

Choose a reason for hiding this comment

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

Suggested change
error_msg <- glue::glue("Couldn't find the file {file_path} in any of the mock directories.")
rlang::abort(error_msg)
rlang::abort(glue("Couldn't find the file {file_path} in any of the mock directories."))

I think this is still under the 100 character lintr line limit, but would be good to confirm

}
3 changes: 2 additions & 1 deletion R/use-dittodb.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
#' }
use_dittodb <- function(path = ".") {
if (!("DESCRIPTION" %in% dir(path))) {
stop(path, " is not an R package directory", call. = FALSE)
error_msg <- glue::glue("{path} is not an R package directory.")
rlang::abort(error_msg)
Comment on lines +34 to +35
Copy link
Collaborator

@jonkeane jonkeane Apr 13, 2024

Choose a reason for hiding this comment

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

Suggested change
error_msg <- glue::glue("{path} is not an R package directory.")
rlang::abort(error_msg)
rlang::abort(glue("{path} is not an R package directory."))

Slightly simpler, what do you think?

}
add_dittodb_to_desc(file.path(path, "DESCRIPTION"))
# TODO: could allow helper.r too
Expand Down
3 changes: 2 additions & 1 deletion R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ get_dbname <- function(dots, drv = NULL) {
path <- "ephemeral_sqlite"
} else {
# if there is no name, or it's empty
stop("There was no dbname, so I don't know where to look for mocks.")
error_msg <- "There was no dbname, so I don't know where to look for mocks."
rlang::abort(error_msg)
Comment on lines +72 to +73
Copy link
Collaborator

@jonkeane jonkeane Apr 13, 2024

Choose a reason for hiding this comment

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

Suggested change
error_msg <- "There was no dbname, so I don't know where to look for mocks."
rlang::abort(error_msg)
rlang::abort("There was no dbname, so I don't know where to look for mocks.")

This should still be under the 100 character lintr limit, and nice to not have the extra line IMO

}
return(db_path_sanitize(path))
}
Expand Down