-
Notifications
You must be signed in to change notification settings - Fork 142
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
Implement r_normalise_encoding()
#1187
Conversation
bb3c8a8
to
044fb22
Compare
Thanks! I added it to the C library under the name Merging now so I can depend on this in the unique branch. |
I like the Based on the naming scheme we used in
|
I try to avoid "normalise" and "standardise" because they are long names and have the "s" vs "z" spelling issue. I think "fixing encoding" could still be documented as producing "an object with standard encoding, meaning that ...". |
Alternatively I considered naming this |
Right, the "unknown" thing is just an implementation detail of UTF-8 strings in R so that shouldn't have an impact on the naming scheme or documentation. I think I like |
Initial vctrs PR: r-lib/vctrs#565 |
Pulled over from https://github.com/r-lib/vctrs/blob/master/src/translate.c
I've left in the maybe-referenced copy semantics, as that seems to work best with vctrs.
Otherwise, I've done minor tweaks to use more of the rlang lib, but nothing major.
I've pulled in all the relevant tests from vctrs, with a few testthat utilities. There are quite a few tests, but I went through each one and none of them feel redundant. I think they are required to ensure we hit all the code paths, but we could move them out of
test-c-api.R
if you feel it clutters it too much.