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.

Sign up for GitHub

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

Jump to bottom

Convert $width to character before calling replace_na() #35

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Convert $width to character before calling replace_na() #35

merged 1 commit into from
Nov 29, 2021

Conversation

DavisVaughan
Copy link
Contributor

We are updating tidyr::replace_na() to utilize vctrs, and that results in slightly stricter / more correct type conversions. See tidyverse/tidyr#1219

We noticed in revdeps that this package broke. An easy way to see this is by installing the PR mentioned above and running:

library(DSSAT)

# Extract file path for sample ecotype file
sample_eco_file <- system.file('extdata','SAMPLE.ECO',package='DSSAT')

# Read sample ecotype file
eco <- read_eco(sample_eco_file)
#> Warning: One or more parsing issues, see `problems()` for details
#> Error: Problem with `mutate()` column `width`.
#> ℹ `width = replace_na(.data$width, "")`.
#> x Can't convert `replace` <character> to match type of `data` <double>.

The problem boils down to the fact that you are calling replace_na(.data$width, replace = ""), but width is a numeric column, not a character column. It looks like you really do want width to be a character column for usage in glue_data() later, so I have preemptively coerced it for you before replace_na() is called.

We would greatly appreciate if you could merge this PR and submit a patch release of your package to CRAN so we can send tidyr in!

@DavisVaughan DavisVaughan mentioned this pull request Nov 18, 2021
Merged
@palderman palderman merged commit 94107e5 into palderman:main Nov 29, 2021
@DavisVaughan
Copy link
Contributor Author

Hi @palderman, were you planning on doing a CRAN release soon? Your package still shows up in our revdepchecks and we are getting closer to release! Thanks!

@palderman
Copy link
Owner

palderman commented Dec 21, 2021 via email

@DavisVaughan DavisVaughan mentioned this pull request Dec 21, 2021
Closed
32 tasks
@DavisVaughan
Copy link
Contributor Author

@palderman it seems like this patch did not make it into your 0.0.5 release. If you download the latest release from CRAN and run the code at the code of this PR then you get the same issue, and it looks like the source code on CRAN doesn't have the patch even though the GitHub code does. Did it get lost somehow?

We plan to release tidyr this week!

@palderman
Copy link
Owner

@DavisVaughan ,🤦Don't know what happened. I'll fix it and get the correct version submitted ASAP.

@DavisVaughan
Copy link
Contributor Author

You may have forgotten to pull in the changes locally on your laptop after merging

@palderman
Copy link
Owner

Proper version has now been submitted to CRAN. Apologies for the delay.

I think what happened is I accepted your pull request into the main branch, but inadvertently submitted the develop branch to CRAN.

In any case, all should be well shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DavisVaughan @palderman