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

Better checking of list arguments to bind_rows() #3068

Merged
merged 6 commits into from Sep 5, 2017

Conversation

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Sep 3, 2017

Fixes #3024.

Related: billdenney/pknca#48 (comment)

@krlmlr krlmlr requested a review from hadley Sep 3, 2017
@lionel-
Copy link
Member

@lionel- lionel- commented Sep 3, 2017

Argument 1 must have names

I think it should be: Argument 1 must be named

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Sep 3, 2017

i.e. lists are treated as data frames so they should have outer names and the inner names of vectors are irrelevant.

Loading

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Sep 3, 2017

To me, in dplyr::bind_rows(a = df), the df is a named argument (because of the name a) but has names itself (the names of the columns. Can you please clarify which call should give which error message?

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Sep 3, 2017

If there are unused outer names it should probably be a warning.

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Sep 3, 2017

oh I understand now: "Argument 1 must have names". Argument 1 refers to the list, got it.

Loading

@lionel-
Copy link
Member

@lionel- lionel- commented Sep 3, 2017

If there are unused outer names it should probably be a warning.

Though the outer names can be used as .id column right? So we should allow unused outer names.

Loading

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Sep 3, 2017

I'm not sure, this PR is just about fixing an annoyance just before the upcoming dplyr bugfix release, which may or may not be connected with a segfault only CRAN is seeing. Can we discuss wording of error messages after the release?

Loading

hadley
hadley approved these changes Sep 5, 2017
Copy link
Member

@hadley hadley left a comment

I think this function would be a bit easier to understand if the first check was to confirm that the input was a vector. But we can do that refactoring later, if needed.

Loading

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Sep 5, 2017

Simplified the check.

Loading

@krlmlr krlmlr requested a review from hadley Sep 5, 2017
hadley
hadley approved these changes Sep 5, 2017
@krlmlr krlmlr merged commit c65368f into tidyverse:master Sep 5, 2017
0 of 2 checks passed
Loading
@krlmlr krlmlr deleted the b-#3024-bind-unnamed branch Sep 6, 2017
@lock
Copy link

@lock lock bot commented Jan 18, 2019

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/

Loading

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants