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

[thisisnic]

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".

[njtierney]

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]

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]

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]

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! :)

[njtierney]

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]

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]

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