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

Remove broken names_ptypes usage #66

Merged
merged 2 commits into from
Dec 29, 2021

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Dec 21, 2021

Hi @atusy,

We are planning a release of tidyr for early to mid January, and your package was flagged in our revdeps.

If you install the dev version of tidyr, you can reproduce the issue with:

x <- data.frame(original = "foo", level1 = "foo", level2 = NA_character_)
ftExtra:::fill_header(x)
#> Error: Can't convert <character> to <integer>.

It looks like in fill_header() you are supplying names_ptypes = integer() in pivot_longer(). This actually should never have worked, you are supposed to supply a list of named ptypes, like list(var = integer()). Regardless, this wouldn't actually work because you can't cast a character column to integer using names_ptypes. If you want that, you need names_transform = list(var = as.integer).

We have just added the ability to do names_ptypes = integer() into the dev version of tidyr, which is why you now see the cast error.

Since you immediately pivot-wider right after this, I don't think you actually need to cast to integer. Just leave them as character names because they will end up as column names in the end.

We would greatly appreciate if you could merge this PR and plan for a CRAN release around early January! You should be able to go ahead and send to CRAN, as this doesn't rely on dev tidyr in any way. Thank you!

@atusy
Copy link
Owner

atusy commented Dec 22, 2021

@DavisVaughan Thank you for the PR and the detailed comments! I really appreciate it. It looks good to me. Anyway, I will try out your change just for sure. I will also plan to release on CRAN as soon as possible.

@atusy atusy merged commit cb96c8a into atusy:master Dec 29, 2021
@atusy
Copy link
Owner

atusy commented Dec 29, 2021

Thanks again!

@atusy
Copy link
Owner

atusy commented Jan 5, 2022

@DavisVaughan FYI, I released ftExtra 0.3.0 on CRAN.

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