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

Replace stopifnot() with assert_that() for better error messages #495

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@katrinleinweber
Copy link
Contributor

commented Apr 8, 2019

Hello :-)

What are your feeling/ideas/suggestions regarding replacing stopifnot() in ep. 10 with assertthat::assert_that()?

This topic came up during a workshop preparation and is related to #221. Over in r-novice-inflammation, we don't teach it, and previous plans to do it were decided against.

This is a quick draft, feedback welcome!

@naupaka

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

We had a discussion about this a few years ago when stopifnot() was added (#221). The sense then was that it was probably better to stick to a simple base R approach to emphasize defensive programming, instead of adding in another dependency since introducing the concept of unit tests (i.e. testthat) in reasonable detail would add a lot of content to already full materials... But perhaps assertthat() is reasonable update? @jcoliver @mawds?

@jcoliver

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

The value of assertthat is in the error messages:

stopifnot:

Error: is.numeric(temp) is not TRUE

assertthat:

Error: temp is not a numeric or integer vector

The syntax between the two is identical, so my question is: does the slightly more informative error message justify the cost of the added dependency?

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

An additional benefit is the reminder to load the library() command. That plus the better readability of errors weighs more, than any risk this "dependency" might introduce. What could the risk be at worst? We're not running a commercial web app here, where each hour downtime causes lost profits ;-)

If this changes proves useful, we could later turn it into an opportunity to introduce the :: operator, or show how install.packages() can handle a vector.

@naupaka

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@katrinleinweber we've (cc @jcoliver @mawds) discussed this and would like to add it as a tip instead of changing the code in the lesson. It is a better function than stopifnot(), but the improvement is not so dramatic to overcome the downside of having another dependency (with the many lessons maintained by the Carpentries and all the automated build systems and different versions of R on Travis, locally and etc flying around it's best to avoid additional complexity when possible.

Would you be willing to update the PR to make it a callout tip instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.