#187: stop() -> rlang::abort() #192
Conversation
Functionize db_path checker. Remove checks from functions that did not need them.
Merge remote-tracking branch 'upstream/main' # Conflicts: # NEWS.md # R/capture-requests.R # man/check_db_path.Rd
| error_msg <- glue::glue( | ||
| "Fixture: {make_path(conn@path, get_type(statement), hash(statement))} | ||
| {clean_statement(statement)}" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.
──────────────────────────────────────────────────────────────────────────────────────────────────
There was a problem hiding this comment.
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
without cli
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for this!
I did a full sweep and things look good. I have a few suggestions for making these single calls. For aborts like these, if they fit within the call I think it's nice to have the message all inside of the call and not need a separate variable where possible — but admit that's a slightly personal choice. I think they will all fit with the lintr, but would be good to confirm (it's run in CI so you don't need to do that locally).
What do you think about also importing abort from rlang? I appreciate not wanting to touch namespace with a new import, but in this case I think it's probably warranted.
Thanks again, I'm excited to see this be in a slightly more modern world :)
| error_msg <- "There was no dbname, so I don't know where to look for mocks." | ||
| rlang::abort(error_msg) |
There was a problem hiding this comment.
| 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
| error_msg <- glue::glue("{path} is not an R package directory.") | ||
| rlang::abort(error_msg) |
There was a problem hiding this comment.
| 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?
| error_msg <- glue::glue("Couldn't find the file {file_path} in any of the mock directories.") | ||
| rlang::abort(error_msg) |
There was a problem hiding this comment.
| 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
#187
stop()call torlang::abort()stop()calls inR/vctrs_s3_register.RDid you want the wording updated in these? The issue mentioned having clearer messages. I wasn't sure if you meant re-wording or rlang itself makes it clearer. Let me know!