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

Fix #101 - throw an error if the x argument supplied to vis_dat is not a data.frame #102

Merged
merged 5 commits into from Nov 28, 2018

Conversation

@thisisnic
Copy link
Contributor

commented Nov 13, 2018

I have updated vis_dat to include a check for if x is an object of class data.frame, and if not throws error "x must be a data.frame object".

R/vis-dat.R Outdated
@@ -64,6 +64,10 @@ vis_dat <- function(x,
warn_large_data = TRUE,
large_data_size = 900000) {

if(!("data.frame" %in% class(x))){

This comment has been minimized.

Copy link
@njtierney

njtierney Nov 14, 2018

Collaborator

Perfect! Can I suggest that you use

! inherits(x, "data.frame)
R/vis-dat.R Outdated
@@ -64,6 +64,10 @@ vis_dat <- function(x,
warn_large_data = TRUE,
large_data_size = 900000) {

if(!("data.frame" %in% class(x))){
stop("x must be a data.frame object")

This comment has been minimized.

Copy link
@njtierney

njtierney Nov 14, 2018

Collaborator

Perhaps we could add something to the message that tells the person what class they provided to visdat as well, so it says something like:

Oop! You have provided me with , but visdat needs a data.frame, perhaps try ...

What do you think?

@njtierney

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

Hello!

This looks great, thanks! :)

I've written a comment on the code there.

How would you feel about adding a test for this?

To get started, you can build on the test code in here,

And then write something like

test_that("vis_dat stops when non data frame provided",{
  expect_error(vis_dat(<unexpected data type here>))
})

Thanks again for your contribution, providing good error messages is hard, and your help is very much valued :)

@thisisnic

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

Thanks for your help and feedback! :)

I've made the updates suggested above (using inherits(), a more user-friendly error message, and adding a test).

I wasn't sure if the error message I added is a little verbose in phrasing - what do you think?

In the example of calling vis_dat(AirPassengers), it would return:
Error: Oops - vis_dat requires x to be an object of class data.frame but the object you've provided me with is of class(es): ts

@njtierney

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

Fantastic, this looks excellent - well done! :)

So writing error messages is hard, and I think that this is great! A few minor changes, I would suggest something like:

vis_dat requires a data.frame, but currently, the object I see has the class/es: class(x)

I changed class(es) because I initially thought that the function class was being applied on the object es.

What do you think? The most important thing is to think about your intial point - the initial error message from visdat wasn't very clear or helpful - do you think that if you saw this error message that you would know what to do to fix it? To me, if an error message can indicate what to do to fix the problem, then that's a win.

Extending this across other parts of visdat.

So, this is actually a really useful feature, that I think should be in all the vis_ functions of visdat!

What do you think about writing this up as a function that can be called in other functions in visdat?

This line of thinking sounded familiar, and it turns out that I have done this in naniar with a function called test_if_dataframe(), which is then called in placed like this.

So, this could be the first line in all the vis_* family of functions.

Of course, I am happy to do this, you have already made a very useful contribution here, but I just thought I'd see if you wanted to continue?

In any case, I'm so glad that you filed the issue #101, I think I'll be more aware of error messages in the future. :)

@thisisnic

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

Thanks again for taking the time to go through this with me!

Yep - I like the phrasing there as it explains everything the end user needs to know without being too wordy or complex. I've updated the code in my PR to include this.

Sounds great, I'd love to! Shall I create a utils.R file in this repo and copy the function across from naniar? Or perhaps it'd fit into internals.R? Or something else? Let me know! :)

R/vis-dat.R Outdated
@@ -66,7 +66,7 @@ vis_dat <- function(x,

# throw error if x not data.frame
if(!(inherits(x, "data.frame"))){
stop("Oops - vis_dat requires x to be an object of class data.frame but the object you've provided me with is of class(es): ",
stop("vis_dat requires a data.frame but the object I see has class/es: ",
paste(class(x), collapse = ", "))

This comment has been minimized.

Copy link
@njtierney

njtierney Nov 19, 2018

Collaborator

Can you add the option for stop, call. = FALSE, as is done in the naniar function :)

@njtierney

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2018

No problems at all - thank you for doing all the work!

PR looks great! I've made a minor comment about using call. = FALSE - this means that the call isn't reported with the error message, I thinks this is a pattern I borrowed from some tidyverse packages.

Good question about where to put it - could you put it in the internals.R file?

Thanks again for your help!

Oh, and make sure to add a bullet to the news.md file to document your great work and contribution, and also add yourself as a contributor in the DESCRIPTION, :) Thanks!

@thisisnic

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

Awesome; I've added the function to internals.R, implemented it in all the main vis_* function, added the relevant test for each function, and added myself to DESCRIPTION. Let me know if there's any other tweaks that need making!

@njtierney

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2018

Wonderful! Thank you very much for this, @thisisnic !

@njtierney njtierney merged commit fca499d into ropensci:master Nov 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.