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

clarify error when rbindlist(l) is not a list() #3638

Merged
merged 5 commits into from
Jun 14, 2019
Merged

clarify error when rbindlist(l) is not a list() #3638

merged 5 commits into from
Jun 14, 2019

Conversation

ajdamico
Copy link
Contributor

hi, in the code below, users hit this error message that says data.table and data.frame objects are OK when they're actually not.. thanks for the amazing software!

library(data.table)


# correct error message
rbindlist( list( 1 ) )
# Error in rbindlist(x) : 
#   Item 1 of input is not a data.frame, data.table or list


# confusing error message, since item 1 is a data.frame
rbindlist( mtcars )
# Error in rbindlist(x) : 
#   Item 1 of input is not a data.frame, data.table or list

class(mtcars)
# [1] "data.frame"

x <- data.table( mtcars )

# confusing error message, since item 1 is a data.table
rbindlist( x )
# Error in rbindlist(x) : 
#   Item 1 of input is not a data.frame, data.table or list

class( x )
# [1] "data.table" "data.frame"


# works as expected
rbindlist( list( mtcars ) )
rbindlist( list( x ) )

hi, in the code below, users hit [this error message](https://github.com/Rdatatable/data.table/blob/2619f1a594fa1e2967c11fb3339522f240e2afa0/src/rbindlist.c#L31) that says `data.table` and `data.frame` objects are OK when they're actually not..  thanks for the amazing software!


	library(data.table)


	# correct error message
	rbindlist( list( 1 ) )
	# Error in rbindlist(x) : 
	#   Item 1 of input is not a data.frame, data.table or list

	
	# confusing error message, since item 1 is a data.frame
	rbindlist( mtcars )
	# Error in rbindlist(x) : 
	#   Item 1 of input is not a data.frame, data.table or list

	class(mtcars)
	# [1] "data.frame"

	x <- data.table( mtcars )

	# confusing error message, since item 1 is a data.table
	rbindlist( x )
	# Error in rbindlist(x) : 
	#   Item 1 of input is not a data.frame, data.table or list

	class( x )
	# [1] "data.table" "data.frame"


	# works as expected
	rbindlist( list( mtcars ) )
	rbindlist( list( x ) )
Copy link
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

Thanks for PR, I left comment how to fix failure of test 682

R/data.table.R Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #3638 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3638      +/-   ##
==========================================
- Coverage   98.19%   98.18%   -0.01%     
==========================================
  Files          66       66              
  Lines       12923    12925       +2     
==========================================
+ Hits        12690    12691       +1     
- Misses        233      234       +1
Impacted Files Coverage Δ
R/data.table.R 97.69% <50%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 421f672...f3109c0. Read the comment docs.

@jangorecki
Copy link
Member

Looks like code there are not unit test for error message added in this PR, could you add one?

@ajdamico
Copy link
Contributor Author

@jangorecki hi, i hope d064de2 works? thanks again

@mattdowle mattdowle added this to the 1.12.4 milestone Jun 14, 2019
@mattdowle mattdowle merged commit f3109c0 into Rdatatable:master Jun 14, 2019
@mattdowle
Copy link
Member

Great. Thanks @ajdamico. I've invited you to be project member so that next time you can create a branch directly in the main project. This makes it easier to work on each other's branches. You'll need to accept the invitation before it is applied (I think it's a notification in the top right of GitHub). Thanks again and welcome!

@mattdowle
Copy link
Member

mattdowle commented Dec 27, 2019

This was a code contribution, so I'm sorry @ajdamico, I should have added you to DESCRIPTION as a contributor at the time. I'll correct my mistake now in a commit to master linking to this PR.
Found as a result of following up on discussion at the end of #1236.
(This contribution was after the license change from GPL to MPL so it doesn't need to be included in the forthcoming retrospective-permission PR.)

@ajdamico
Copy link
Contributor Author

ajdamico commented Jan 5, 2020

we use this software every day. thank you!!

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.

3 participants