Use cli error chaining + add clickable link to bad config #2602
Conversation
… basename, but still create a valid hyperlink
R/lint.R
Outdated
| x = "Instead, it was {.val {column_number}}." | ||
| )) | ||
| cli_abort( | ||
| "{.arg column_number} must be an integer between \\ |
There was a problem hiding this comment.
please don't use the end-of-line \\, I find the behavior pretty confusing & it's not a well-documented part of the R parser.
There was a problem hiding this comment.
Note that this repo uses line width=120, so you also don't need to squeeze your code so tightly :)
There was a problem hiding this comment.
@MichaelChirico Out of curiosity, what's the kosher way of doing readable, multiline strings? That is, one that doesn't use \\?
Because this is what I do as well 😅
There was a problem hiding this comment.
I ended up not putting \\ but still keeping line break. cli doesn't care about line breaks (or double spaces) like glue does. With cli, you have to put f to force a line break.
Lmk if you want another readjustment!
There was a problem hiding this comment.
Out of curiosity, what's the kosher way of doing readable, multiline strings?
paste(), paste0()
R/settings.R
Outdated
| conditionMessage(e) | ||
| )) | ||
| cli_abort( | ||
| "Error from {config_link} setting {.code {setting}} in {.code {format(conditionCall(e))}}:", |
There was a problem hiding this comment.
Is format(conditionCall(e)) redundant with parent=e?
There was a problem hiding this comment.
I had not looked closely enough at this one.
Here is what it looks like with my changes
I think it could still read a bit better
(Fr missing argument with no default
There was a problem hiding this comment.
Agree it's nicer to surface 'lint_dir()' as the error source up front.
There was a problem hiding this comment.
Oh, sorry. In 07a96a0, I changed this back to Error in read_settings() : since read_settings() is exported by lintr. I could add a call argument to read_settings() if it makes sense to do so, but I didn't want to change anything in the user-facing functions. I can open an issue to do this in a follow-up? Or do it in this PR? Thanks for the second review btw
There was a problem hiding this comment.
Hmm, read_settings() is not exported though 🤔
There was a problem hiding this comment.
Oh, okay! It is just a forgotten #' @noRd that triggered build failure. I will add this and unrevert my change.
|
Thanks for the PR! I'll let @IndrajeetPatil add final review. |
* Pass call to the malformed error message (to show Error in `lint()`: instead of Error in `read_config_file()`: * Remove conditionCall(e) since we use `parent = e`
… argument. Config error will therefore read as Error in `read_settings()`: instead of Error in `lint()`:
olivroy
changed the title
Co-authored-by: Michael Chirico <[email protected]>
R/with.R
Outdated
| if (missing(defaults) || !is.list(defaults) || !all(nzchar(names2(defaults)))) { | ||
| cli_abort("{.arg defaults} must be a named list.") | ||
| if (missing(defaults)) { | ||
| cli_abort("Missing required argument {.arg defaults} should be a named list.") |
There was a problem hiding this comment.
I think this error message should only mention that the required argument is missing.
"Missing required argument" sounds a bit odd, and combines the error message for two different checks into one.
cli_abort("{.arg defaults} is a required argument, but is missing. [It should be a named list.]") # second sentence is optionalThere was a problem hiding this comment.
I agree, it would mimic more what rlang::check_required() provides. I can make this change. and adjust test.
! `x` is absent but must be supplied.When you see this message, you either know what to provide instead, or can look at the documentation
R/settings.R
Outdated
| config_link <- cli::style_hyperlink( # nolint: object_usage_linter. TODO(#2252). | ||
| cli::col_blue(basename(config_file)), | ||
| url = paste0("file://", config_file) | ||
| ) |
There was a problem hiding this comment.
Should we create a helper to create these clickable links?
We are already doing it twice in settings.R, and maybe will need to do it a few more times in future, and I don't wish for these nolint directives to proliferate.
There was a problem hiding this comment.
Moved it to helper. However, still need the nolint directive until #2252 is fixed. It is that the variable created in unused, except in the error message.
The nolint can only be removed if the function is called directly in the cli message
fix #2588, follows-up to #2418
Thanks @IndrajeetPatil for merging #2418. I think it is a good improvement.
Taking advantage of this to send this up.
Basically uses
parentfrom cli to format error messages.Add a link to config file.
Tweak some wordings that didn't seem clear to me and use https://style.tidyverse.org/error-messages.html as a reference.
Screenshots of how this looks: (I had more but I erased my console output) (config is a clickable link to .lintr)
Changed wording of this one, but made the note about the warning that will disappear a little less prominent
Removed some
{.cls}. Imo, it decreases readability, by adding<>, I think it reads better as a sentence.