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

str_detect keyword arguments generate errors #444

Closed
kmishra9 opened this issue May 7, 2020 · 5 comments
Closed

str_detect keyword arguments generate errors #444

kmishra9 opened this issue May 7, 2020 · 5 comments

Comments

@kmishra9
Copy link

@kmishra9 kmishra9 commented May 7, 2020

Code that ran fine under past versions of dbplyr threw some errors. I'm assuming the changes made in the newest dbplyr changelog (1.4.3) probably created the issue?

library("dplyr", warn.conflicts = FALSE)

lf <- dbplyr::lazy_frame(x = "0.88", con = dbplyr::simulate_postgres())

lf %>% mutate(has_88 = str_detect(string = x, pattern = "88"))
#> <SQL>
#> Error in str_detect(string = x, pattern = "88"): unused arguments (string = x, pattern = "88")

lf %>% mutate(has_88 = str_detect(x, "88"))
#> <SQL>
#> SELECT `x`, `x` ~ '88' AS `has_88`
#> FROM `df`

Created on 2020-05-07 by the reprex package (v0.3.0)

@kmishra9 kmishra9 changed the title str_detect positional arguments generate errors str_detect keyword arguments generate errors May 7, 2020
@kmishra9
Copy link
Author

@kmishra9 kmishra9 commented May 8, 2020

Including a value for the negate argument (even without a keyword) also appears to fail with the same "unused argument" issue as above

library("dplyr", warn.conflicts = FALSE)

lf <- dbplyr::lazy_frame(x = "0.88", con = dbplyr::simulate_postgres())

lf %>% mutate(has_88 = str_detect(string = x, pattern = "88"))
#> <SQL>
#> Error in str_detect(string = x, pattern = "88"): unused arguments (string = x, pattern = "88")

lf %>% mutate(has_88 = str_detect(x, "88"))
#> <SQL>
#> SELECT `x`, `x` ~ '88' AS `has_88`
#> FROM `df`

lf %>% mutate(not_88 = str_detect(x, "88", FALSE))
#> <SQL>
#> Error in str_detect(x, "88", FALSE): unused argument (FALSE)

Created on 2020-05-08 by the reprex package (v0.3.0)

@hadley hadley closed this as completed in d6c1713 May 15, 2020
@hadley
Copy link
Member

@hadley hadley commented May 15, 2020

Generally, I think it's bad practice to name the first ~2 arguments because you can generally expect the reader to know what they are if they recognise the function name.

@kmishra9
Copy link
Author

@kmishra9 kmishra9 commented May 16, 2020

As one of the readers of my old code, I tend to disagree with that principle, but even ignoring the semantics of that, its still... broken, no? And the fact that it doesn't take a third argument (or accept the third argument with a message that it will be ignored) indicates an issue as well?

More importantly, I think it's a "breaking change" where code that's part of a production pipeline no longer works as written if keywords then have to be removed...

@hadley
Copy link
Member

@hadley hadley commented May 16, 2020

Yes, that's why I fixed it.

@kmishra9
Copy link
Author

@kmishra9 kmishra9 commented May 16, 2020

Oops -- thought it was closed as irrelevant, my bad, didn't see the attached commit

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

No branches or pull requests

2 participants