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

Add replace_na.default for vectors (alternative approach) #342

Closed
wants to merge 2 commits into from

Conversation

flying-sheep
Copy link

@flying-sheep flying-sheep commented Aug 10, 2017

I saw #258, but didn’t like the idea of using a single-element list.

This version is natural, but not type-stable. (replace is expected to be a list when data is a data.frame, else a vector)

There’s another approach, which is removing “replace” and using ... (backwards-compatible):

replace_na.default <- function(data, ...) {
  args <- list(...)
  stopifnot(length(args) == 1L)
  stopifnot(is_null(names(args)))
  replace <- args[[1L]]
  …
}
replace_na.data.frame <- function(data, ...) {
  replace <- list(...)
  if (length(replace) == 1L && is_list(replace[[1L]]))
    replace <- replace[[1L]]  # old signature
  …
}

Please tell me what you’d rather like to see.

@hadley
Copy link
Member

hadley commented Nov 15, 2017

It's been a while since you submitted so I wanted to check before starting the review process: are you interested in finishing this off?

@flying-sheep
Copy link
Author

Sure, let’s go! I might not respond to comments before sunday though.

R/replace_na.R Outdated
replace_na.data.frame <- function(data, replace = list(), ...) {
replace_na.default <- function(data, replace = NULL, ...) {
if (is_null(replace))
stop("If 'data' is a vector, 'replace' has to be provided.")
Copy link
Member

Choose a reason for hiding this comment

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

Why?

I think instead it would be better to assert that replace has length 1

Copy link
Author

Choose a reason for hiding this comment

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

Well, the two things that that can happen are the following. Pretty clear message:

> data[1:5] <- NULL
Error in data[1:5] <- NULL: replacement has length zero
> data[1:5] <- 1:3
Warning message in data[1:5] <- 1:3:number of items to replace is not a multiple of replacement length

If this now happens through our function here, we get:

> replace_na(data)
Error in data[are_na(data)] <- replace: replacement has length zero
> replace_na(data, 1:3)
Warning message in data[are_na(data)] <- replace:number of items to replace is not a multiple of replacement length

Of which I only think the last one is clear.

Copy link
Member

Choose a reason for hiding this comment

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

But this might also happen: data[1:6] <- 1:3. We want to avoid accidentally recycling the replace value

Copy link
Author

Choose a reason for hiding this comment

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

OK, done!

R/replace_na.R Outdated
if (length(replace) != 1L)
stop("If 'data' is a vector, 'replace' has to be of length 1, not ", length(replace))

data[are_na(data)] <- replace
Copy link
Author

Choose a reason for hiding this comment

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

maybe I should use replace[[1L]] here to allow people to call replace_na(vec, list(5))?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea! It's more consistent with the rest of the API. (But you'll need to check that replace() is a length 1 list)

Copy link
Author

Choose a reason for hiding this comment

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

We require it to be length 1 anyway, so no matter if list or not, replace[[1L]] will always be correct, no?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to be strict about the types here

Copy link
Author

Choose a reason for hiding this comment

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

OK, done!

@hadley hadley closed this in bd62a4f Nov 21, 2017
@hadley
Copy link
Member

hadley commented Nov 21, 2017

I ended up going in a slightly different direction; as well as adding a better error message for the data frame case, and adding support for list-cols. Sorry I didn't merge your PR. I really appreciate the effort 😁

@flying-sheep flying-sheep deleted the patch-1 branch November 21, 2017 15:42
@flying-sheep
Copy link
Author

no worries!

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.

None yet

2 participants