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

Propagate name parameter in match_value. #175

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Conversation

plietar
Copy link
Member

@plietar plietar commented Sep 4, 2024

Without this, when the first argument of match_value was not a valid character scalar, the function would unconditionally call it x, even if the caller's code makes no mention of x.

match_value(5, list("foo", "bar"))
#> Error:
#> ! 'x' must be character

With this change, the name as used in the caller of match_value is used, which gives a less confusing output:

match_value(5, list("foo", "bar"))
#> Error:
#> ! '5' must be character

@plietar plietar requested a review from richfitz September 4, 2024 16:28
@plietar
Copy link
Member Author

plietar commented Sep 4, 2024

@richfitz do you know what the arg that gets passed around throughout the file is for? It just gets passed to cli::cli_abort but I don't see any mention of it in the cli docs.

Without this, when the first argument of `match_value` was not a valid
character scalar, the function would unconditionally call it `x`, even
if the caller's code makes no mention of `x`.

```r
match_value(5, list("foo", "bar"))
#> Error:
#> ! 'x' must be character
```

With this change, the name as used in the caller of `match_value` is
used, which gives a less confusing output:

```r
match_value(5, list("foo", "bar"))
#> Error:
#> ! '5' must be character
```
@richfitz richfitz merged commit f580aff into main Sep 5, 2024
10 checks passed
@richfitz
Copy link
Member

richfitz commented Sep 5, 2024

See ?rlang::abort, where it is used - in the error it will highlight the offending argument (I think you will see that with rlang::last_trace or rlang::last_error.

I've tidied a few of these things up as https://github.com/reside-ic/reside.utils/ which we can pull in for reuse in orderly among others, I'll get a PR together for this perhaps on the train on the way home tonight

richfitz added a commit to reside-ic/reside.utils that referenced this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants