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

Review all error messages #659

Closed
8 tasks done
krlmlr opened this issue Oct 1, 2019 · 10 comments · Fixed by #731
Closed
8 tasks done

Review all error messages #659

krlmlr opened this issue Oct 1, 2019 · 10 comments · Fixed by #731
Labels
vctrs ↗️
Milestone

Comments

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Oct 1, 2019

Refactor

  • test against known output
  • use classed conditions
  • use attributes for classed conditions

Add new

  • expect_error_relax()
  • existing examples in "invariants"
  • use verify_output() for invariants
  • usages that are not explicitly allowed in "invariants"
  • warnings
@lionel-
Copy link
Member

@lionel- lionel- commented Oct 8, 2019

[[ can now use vctrs::vec_as_position():

vctrs::vec_as_position(100, length(mtcars), names = names(mtcars))
#> Error: Must index existing elements.
#> x Can't subset position 100.
#> i There are only 11 elements.

vctrs::vec_as_position("foo", length(mtcars), names = names(mtcars))
#> Error: Must index existing elements.
#> x Can't subset element with unknown name `foo`.

vctrs::vec_as_position(new.env(), length(mtcars), names = names(mtcars))
#> Error: Must extract with a single position or name.
#> x `i` has the wrong type `environment`.
#> i Positions and names must be integer or character.

vctrs::vec_as_position(factor("cyl"), length(mtcars), names = names(mtcars))
#> [1] 2

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Oct 8, 2019

Thanks. Is there a way to replace "elements" by "columns" in the error messages? I remember a solution to a similar problem in tidyselect.

@lionel-
Copy link
Member

@lionel- lionel- commented Oct 8, 2019

We have decided to use a generic "element" for simplicity (for now at least).

@lionel-
Copy link
Member

@lionel- lionel- commented Oct 15, 2019

You can now override cnd_issue() to mention columns and data frames. See https://github.com/r-lib/tidyselect/pull/124/files#diff-a6d08219ee9b1f34a3ebbdedbe63b321 for an example.

The issue message should probably be as consistent as possible as the parent error's.

@krlmlr krlmlr added the vctrs ↗️ label Oct 27, 2019
@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Nov 2, 2019

Relevant: tidyverse/design#100.

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Nov 2, 2019

@lionel-: If we create a condition from a parent condition, is it good practice to copy over the detail information from the parent condition (such as location)?

krlmlr added a commit that referenced this issue Nov 2, 2019
- Using classed conditions. All classes start with `"tibble_error_"` and also contain `"tibble_error"` (#659).
krlmlr added a commit that referenced this issue Nov 2, 2019
- Add proper error messages everywhere, avoid "could not find function error_xxx()" errors (#659).
@lionel-
Copy link
Member

@lionel- lionel- commented Nov 4, 2019

I think the better practice is to implement bullets methods that forwards to the parent. But we are only discovering these things :)

We don't have a cnd_details() method yet but we will in the next rlang version.

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Nov 4, 2019

Thanks. I'd rather not have any methods for condition classes, they clutter NAMESPACE. The formula/function interface is more than enough for this use case I think. (And these are very easy to copy.)

@lionel-
Copy link
Member

@lionel- lionel- commented Nov 5, 2019

I don't think cluttering NAMESPACE should be a concern in any circumstance though. Unlike the pkgdown reference, which is easy to free of clutter.

@github-actions
Copy link

@github-actions github-actions bot commented Mar 22, 2021

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
vctrs ↗️
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants