Closes #175 Adds assertions to exported functions

[averissimo]

• Closes Add {checkmate} dependency for function argument checks #175

Changes Description

• Adds dependency checkmate
• Asserts parameters of all exported functions
• Standard error message for complex assertions (non-ascii characters, ...)
• Changes default parameters to retrieve data.frame attributes (domain, metadata) instead of NULL

Technical list

• Add assertions to all exported functions
• Leverage checkmate::test_* for consistent tests
• Add documentation to wiki
• Keep test coverage at 100%

Task List

• The spirit of xportr is met in your Pull Request
• Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• Summary of changes filled out in the above Changes Description. Can be removed or left blank if changes are minor/self-explanatory.
• Check that your Pull Request is targeting the devel branch, Pull Requests to main should use the Release Pull Request Template
• Code is formatted according to the tidyverse style guide. Use styler package and functions to style files accordingly.
• Updated relevant unit tests or have written new unit tests. See our Wiki for conventions used in this package.
• Creation/updated relevant roxygen headers and examples. See our Wiki for conventions used in this package.
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Run pkgdown::build_site() and check that all affected examples are displayed correctly and that all new/updated functions occur on the "Reference" page.
• Update NEWS.md if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
• Address any updates needed for vignettes and/or templates
• Link the issue Development Panel so that it closes after successful merging.
• Fix merge conflicts
• Pat yourself on the back for a job well done! Much love to your accomplishment!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d6daff2) 100.00% compared to head (9836f8b) 100.00%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #190   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          576       613   +37     
=========================================
+ Hits           576       613   +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[averissimo]

This should be merged after #193 as assertion on .df can be safely moved to group of functions.

Base branch has changed to 182-remove-df-expr. Once #193 is merged it will automatically change to main

[averissimo]

It can be reviewed though as it does not depend on any functionality from #193

[bms63]

Could we get a brief blurb in the news about using the checkmate package now in our code. I know it isn't really user-facing, but I think this points to us being more rigorous in our code.

[bms63]

Hi @averissimo when you get a chance - can you look into these merge conflicts.

[averissimo]

@bms63 Merged and ready for review 😁

Notable topic for discussion:

• Default parameter better reflects internal logic
  • attr(.df, "_xportr.<name>_") instead of NULL

[bms63]

@bms63 Merged and ready for review 😁

Notable topic for discussion:

• Default parameter better reflects internal logic

  • attr(.df, xportr.) instead of NULL`

Looking over PR this makes sense to me now. Perhaps we should explain this a bit more in the argument. A lot of R users are not so familiar with the attr function and what it is doing for us.

[bms63]

This looks great to me! Thanks for sticking with it @averissimo!! @elimillera or @vedhav anything else to add?? The NULL removal is a big change to our documentation and I think warrants an explanation. I think we should do the documentation in a separate issue/PR as this one is a big update already if you all do not object.

[EeethB]

Thanks for contributing, I bet this will make errors clearer! I'm good with merging once we sort out the default parameter values

[bms63]

@averissimo is it possible to revert the arguments back to NULL and sort this out in another issue/PR. I think this impacts some of the stuff @EeethB is working on

[averissimo]

@bms63 @EeethB reverted and cleaned it up a bit. Let me know if I can merge it 😄

[bms63]

LGTM!