Delegate name repair to vctrs (#608).#608
Conversation
🎉🎉🎉 |
jennybc
left a comment
jennybc
left a comment
There was a problem hiding this comment.
I feel like it is rather important to report which names we find problematic. I've always been irritated by errors where the function throwing it knows perfectly well where the problem is, but refuses to reveal that to me.
How hard is it to improve the error messages? I haven't watched the implementation over in vctrs closely enough to know this off-hand.
|
Should |
|
The names repairs are reported in vctrs @jennybc. I think Kirill was referring to other kinds of error messages. |
| expect_equal( | ||
| error_column_names_must_be_unique("a", repair = FALSE), | ||
| "Column name `a` must not be duplicated." | ||
| "Names must be unique." |
There was a problem hiding this comment.
@lionel I thought he was talking about this sort of change: where before we said exactly which name was duplicated, but after we just say that something is duplicated. And the user has to go figure out what it is.
There was a problem hiding this comment.
Oh right. We don't have a good protocol for customising error messages right now, short of catching and rethrowing (which can be expensive).
There was a problem hiding this comment.
This could be dealt with as part of r-lib/vctrs#401. If vctrs returned a classed condition with attributes, tibble could take care of the custom formatting.
krlmlr
force-pushed
the
f-464-name-repair
branch
from
7d592a2 to
735cf94
Compare
|
I'll remove the cruft as part of #609. This PR is meant to be as lean as possible. I have adapted the vctrs pull request to return classed conditions for name repair, so that we can continue throwing custom error messages. |
|
Nice! |
krlmlr
changed the title
3 participants
Closes #464.
This works out of the box with all tests passing, thanks @lionel-!
We are losing the detailed error messages, though. Not sure how important they are for us.