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

Using 'assertive' introduces unnecessary dependencies #149

Closed
echasnovski opened this Issue Jul 19, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@echasnovski
Copy link
Collaborator

echasnovski commented Jul 19, 2018

Recently I installed infer for the first time and noticed that this also got 16 (sixteen!) other assertive.* packages installed. Those now occupy most of the 'Packages' tab when I open it. This is (weirdly) somewhat annoying as I have to do extra scrolling to get to the package I need.

After studying internal code I saw that assertive is only used for checking types of input arguments. In other words, it is not crucial for the package logic but rather a convenient implementation choice. I can offer to change this implementation to one of the following:

  1. Use assertive.types package directly. This reduces number of assertive.* packages to 2 and is enough for current code. However, other assertion types may introduce the need for other assertive.* packages.

  2. Use some other (more generic) package for type checks. I can recommend checkmate for this. This reduces number of extra packages to constant 2 (checkmate and its dependency backports). It is also flexible, easy to use and extend, and has built-in functions for all current type checks (right now no extending is needed). Error messages are very similar to assertive: both provide present and needed types:

    x <- TRUE
    
    assertive::assert_is_numeric(x)
    #> Error: is_numeric : x is not of class 'numeric'; it has class 'logical'.
    
    checkmate::assert_numeric(x)
    #> Error in checkmate::assert_numeric(x): Assertion on 'x' failed: Must be of type 'numeric', not 'logical'.
  3. Write own assertion functions. For the current code not much work is needed to implement this. The distinctive advantage of this approach is the ability to customize error messages: for example, remove those "Error ..." in the beginning. No extra packages are needed for this option (rlang should be enough).

I am wondering the following:

  • Would you be interested in PR regarding updating assertions implementation?
  • If yes, which type of change would you prefer?
@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 21, 2018

@echasnovski Feel free to create a PR to write the checks at every place the assertive package is used to write customizable checks. @richierocks may also be able to provide some guidance here as the assertive package author in addition to his pathological package update here.

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 21, 2018

We wrote many of the check functions on our own as you can see by scanning through the code. Some of them were a bit overkill though so we went to using assertive instead.

@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Jul 21, 2018

If writing own check functions is a way to go then I have a suggestion to use dplyr approach in handling errors (see dplyr/error.R). In this case it means to copy glubort() function which is a refined variant of stop(paste(...)). It might (eventually) replace all assertive and stop(paste(...)) usage. This also means adding glue package to 'Imports' (with no extra implicit dependencies). Its usage can improve code readability in cases of paste()ing strings with data (not related to error handling).

I will write a PR with this approach to replace all assertive assertions. If this will be accepted I can make a new PR to fully use glue.

Does it sound good? If not, then I'll just write code without glue, no problem.

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Jul 21, 2018

Using {glue} in a PR sounds great. It was on my to-do list to do so throughout the package after seeing Jim Hester’s talk at useR about it.

ismayc added a commit that referenced this issue Jul 23, 2018

Merge pull request #152 from echasnovski/check-type
Stop using package {assertive} in favor of custom type checks (#149)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment