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

Make coerce_to_numeric() internal #226

Closed
wants to merge 26 commits into from

Conversation

IndrajeetPatil
Copy link
Member

@IndrajeetPatil IndrajeetPatil commented Aug 20, 2022

Closes #206

I will update NEWS.md later.

@IndrajeetPatil IndrajeetPatil marked this pull request as ready for review August 20, 2022 16:15
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2022

Codecov Report

Merging #226 (504fd2b) into main (f9c1ac2) will not change coverage.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##             main     #226   +/-   ##
=======================================
  Coverage   88.01%   88.01%           
=======================================
  Files          58       58           
  Lines        4181     4181           
=======================================
  Hits         3680     3680           
  Misses        501      501           
Impacted Files Coverage Δ
R/to_numeric.R 90.32% <ø> (ø)
R/data_restoretype.R 90.90% <66.66%> (ø)
R/data_to_long.R 93.33% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@strengejacke strengejacke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work, we need to keep coerce_to_numeric() at least as internal.

See data_to_long(mtcars, 2:4).

@IndrajeetPatil
Copy link
Member Author

It's worrisome that not a single test failed 😐

@IndrajeetPatil IndrajeetPatil marked this pull request as draft August 21, 2022 07:14
@IndrajeetPatil IndrajeetPatil changed the title Remove coerce_to_numeric() WIP: Make coerce_to_numeric() internal Aug 21, 2022
@strengejacke
Copy link
Member

It's worrisome that not a single test failed 😐

Yes, I added a test that should capture this change (hopefully)

@strengejacke
Copy link
Member

I'm not sure about data_restoretype(), though, how to add a test that detects if as.numeric() will work or not.

@strengejacke
Copy link
Member

This would be the new behaviour with as.numeric():

library(datawizard)
data <- data.frame(
  Sepal.Length = c("1", "3", "a"),
  Species = c("setosa", "versicolor", "setosa"),
  New = c("1", "3", "4")
)

fixed <- data_restoretype(data, reference = iris)
#> Warning in data_restoretype(data, reference = iris): NAs introduced by coercion
fixed
#>   Sepal.Length    Species New
#> 1            1     setosa   1
#> 2            3 versicolor   3
#> 3           NA     setosa   4

and this the old behaviour, when we keep coerce_to_numeric() as internal:

library(datawizard)
data <- data.frame(
  Sepal.Length = c("1", "3", "a"),
  Species = c("setosa", "versicolor", "setosa"),
  New = c("1", "3", "4")
)

fixed <- data_restoretype(data, reference = iris)
fixed
#>   Sepal.Length    Species New
#> 1            1     setosa   1
#> 2            3 versicolor   3
#> 3            a     setosa   4

@DominiqueMakowski what would you say is the expected behaviour?

@strengejacke
Copy link
Member

(we should add the example of the expected behaviour as test)

@strengejacke
Copy link
Member

@DominiqueMakowski what would you say is the expected behaviour?

bump 😬

@IndrajeetPatil
Copy link
Member Author

bump

I want to make a new release soon, since I want the JOSS paper to be based on the more stable API of 0.6.0 instead of the current 0.5.0 release API.

Just so you know in case you want this PR to be part of the next release.

@strengejacke
Copy link
Member

@DominiqueMakowski what would you say is the expected behaviour?

bump 😬

hello-mcfly

@IndrajeetPatil IndrajeetPatil changed the title WIP: Make coerce_to_numeric() internal Make coerce_to_numeric() internal Sep 19, 2022
@IndrajeetPatil IndrajeetPatil marked this pull request as ready for review September 19, 2022 15:58
@IndrajeetPatil
Copy link
Member Author

Damn it. Merge conflict again.

@strengejacke
Copy link
Member

@DominiqueMakowski

@DominiqueMakowski what would you say is the expected behaviour?

bump 😬

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.

Consolidate coerce_to_numeric() into to_numeric() via a new argument
3 participants