lint package and add asserthat

[kieranjmartin]

resolves #92 and #95

[github-actions[bot]]

Code Coverage Summary

Filename                Stmts    Miss  Cover    Missing
--------------------  -------  ------  -------  ------------
R/ascii_tables.R           85       3  96.47%   9, 126, 154
R/cast_variables.R         49       0  100.00%
R/diffdf.R                137      18  86.86%   282-299, 326
R/generate_keyname.R       10       1  90.00%   15
R/identify.R              128       9  92.97%   31, 266-273
R/is_different.R           52       0  100.00%
R/issuerows.R              40       0  100.00%
R/issues.R                 13       1  92.31%   47
R/misc_functions.R         14       2  85.71%   8, 12
R/print.R                  23       2  91.30%   42-43
TOTAL                     551      36  93.47%

Results for commit: 1f69fde5155267e521ead34ef6c7025aa1d816bb

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[github-actions[bot]]

Unit Tests Summary

  1 files   10 suites   5s :stopwatch:  38 tests  38 :white_check_mark: 0 :zzz: 0 :x: 564 runs  564 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 1f69fde5.

:recycle: This comment has been updated with latest results.

[kieranjmartin]

@gowerc I would recommend looking at this commit https://github.com/gowerc/diffdf/pull/96/commits/11e47ec387ac47ecd28f99665b5a01d4fadef87a for the assertthat changes in particular. One thing I wasn't sure on is that testthat recommends not depending on the exact message from different packages (such as assert that) as that can cause issues during version changes.

The problem is I don't think when testing an assertion, just checking for an error seems insufficient to me (most of the things assertions check for are likely to cause errors even if the assertion don't catch them). The alternative would be to give a customised error message each time, or just trust that assert that messages will remain consistent over time.

[gowerc]

@kieranjmartin - Regarding the assertthat messages, I'm not really sure what the best solution here is. On other projects we've just expected a single word or 2 in the error message to try and balance the ability to change the error message without being a blanket catch all errors e.g. something like:

assertthat(
    is.numeric(y)
    msg = "`y` must be a numeric"
)

expect_error(...,  regex = "`y`")

[kieranjmartin]

@gowerc updates made. As a note we currently have identical assertions in the print function and the get_table function; I personally think assertions should only be on user facing functions, which would be more consistent with the code elsewhere (no other internal function has assertions)