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

combine(NULL) should return logical() instead of error #3365

Closed
zeehio opened this issue Feb 18, 2018 · 8 comments
Closed

combine(NULL) should return logical() instead of error #3365

zeehio opened this issue Feb 18, 2018 · 8 comments
Labels

Comments

@zeehio
Copy link
Contributor

@zeehio zeehio commented Feb 18, 2018

Given that combine() acts like c() I don't understand why combine() (without args) does not work like c() returning NULL and raises an error instead.

If I am combining a list that may contain all NULL elements, I'd rather get back a NULL than tryCatching or checking for that condition myself in advance. It's not a big deal, but it does not seem consistent with for instance bind_rows() that returns an empty tibble.

If you agreed I'd be happy to provide a pull request.

c(1, NULL, 2)
#> [1] 1 2
dplyr::combine(1, NULL, 2)
#> [1] 1 2
c()
#> NULL
dplyr::combine(NULL)
#> Error in combine_all(args): no data to combine, all elements are NULL
c(NULL, NULL)
#> NULL
dplyr::combine(NULL, NULL)
#> Error in combine_all(args): no data to combine, all elements are NULL

As always, thanks for your time

zeehio added a commit to zeehio/dplyr that referenced this issue Feb 18, 2018
@krlmlr
Copy link
Member

@krlmlr krlmlr commented Feb 28, 2018

@romainfrancois: Do you remember why you made combine() raise an error in response to #1596? I feel this and combine(NULL) or combine(list()) should return logical(), for consistency with tibble::enframe(NULL) .

@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Feb 28, 2018

I don’t recall. Why logical ?

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Feb 28, 2018

Because logical() can be coerced to anything, see also tidyverse/tibble#352.

@romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Mar 1, 2018

That makes sense. Not sure why we went for strict error.

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Mar 1, 2018

Thanks.

@zeehio: Would you like to submit a PR?

@zeehio
Copy link
Contributor Author

@zeehio zeehio commented Mar 1, 2018

I will be happy to submit a PR next week, if you don't mind the delay 😃

zeehio added a commit to zeehio/dplyr that referenced this issue Mar 1, 2018
@zeehio zeehio changed the title combine(NULL) should return NULL instead of error combine(NULL) should return logical() instead of error Mar 1, 2018
@zeehio
Copy link
Contributor Author

@zeehio zeehio commented Mar 1, 2018

@krlmlr submitted, feel free to review whenever you have the time 😄 #3387

@lock
Copy link

@lock lock bot commented Sep 8, 2018

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Sep 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants