search

moodymudskipper / inops

Infix Operators for Detection, Subsetting and Replacement
GNU General Public License v3.0
40 stars 0 forks source link

Improve error "NAs are not allowed in subscripted assignments" in replacement functions #21

Open karoliskoncevicius opened 4 years ago

karoliskoncevicius commented 4 years ago

I was working with examples in the help files and stumbled upon this by accident:

x <- c(1, NA, 2)
x %out[]% 2 <- x

Error in x[list] <- values :
  NAs are not allowed in subscripted assignments

This is of course a non-intended usage. But it's a bit unintuitive to receive this message. More so because if x didn't have missing values - it would work:

x <- c(1, 1.5, 2)
x %out[]% 2 <- x

Warning message:
In x[list] <- values :
  number of items to replace is not a multiple of replacement length

x
[1] 1.0 1.5 2.0
karoliskoncevicius commented 4 years ago

@moodymudskipper pinging for opinions.

moodymudskipper commented 4 years ago

It's consistent with replace, and to me seems like the right behavior but we could override the error message with a better one.

karoliskoncevicius commented 4 years ago

On a second thought - maybe this also should be left for possible future adjustments. We are close to having a first workable version, so no need to complicate things further until then...

moodymudskipper commented 4 years ago

The translation of the first error is something like this :

The ouput of x %out[]% interval contains undetermined elements (shown as NA values), so in turn the number of the elements to replace is unknown. However you tried x %out[]% interval <- value with a value of length superior to 1 that cannot be assigned or recycled unambiguously.

Your english is better than mine so maybe you'll have a better wording

moodymudskipper commented 4 years ago

It wouldn't be that bad to make this warning fail though... As much as I like consistency with base, I think this is never useful, you can have a script running for 20 min, only to spit when it will crash a collection of these warnings, with an intimidating "type warnings() to see the first 50". Maybe better cut it at the source.