Add initial tests: take 2

[cjrace]

Take 2 of PR #5.

Overview

This adds a number of bits of testing infrastructure into the package, and does some initial clean up too.

Details

1. Update license by running the command locally to create appropriate files
2. Added r cmd check, main method for checking the package using CI
3. Added code coverage CI checks and link to codecov.io
4. Added myself as an author / maintainer
5. Updated roxygen2 version (now it will build documentation as a part of the package check)
6. Added validation to support_panel(), more could be done
7. Added some unit tests to support_panel(), these are not exhaustive
8. Fixed documentation and linting errors flagged by package checks
9. Added notes to the readme to help future contributors

Additional information

Have raised issues #20, #21, #22 , #23, #24, #26, #27 off the back of this. To be addressed in separate PRs.

[cjrace]

Current overview from code coverage incase it's interesting. Means if this is merged as is, we'll get a red badge with 43.75% on.

[rmbielby]

Tests all seem to work fine. Comments are mainly just tidying up some mistakes that I'd made.

[rmbielby]

Don't think we need these library calls here... need them listed under imports in the function header.

[cjrace]

Interesting - happy to help mop this up but not sure if blank importing whole packages for each function in our package is the best way - https://roxygen2.r-lib.org/articles/namespace.html.

It is possible, but not generally recommended to import all functions from a package with @import package. This is risky if you import functions from more than one package, because while it might be ok today, in the future the packages might end up with a function having the same name, and your users will get a warning every time your package is loaded.

Reading that I'd be tempted to either do the @import , or just bite the bullet of explicit pkg::function() calls. All the packages mentioned are already in the description file as far as I can see.

Interested to know your thoughts @rmbielby?

[rmbielby]

Interesting - happy to help mop this up but not sure if blank importing whole packages for each function is the best way - https://roxygen2.r-lib.org/articles/namespace.html.

It is possible, but not generally recommended to import all functions from a package with @import package. This is risky if you import functions from more than one package, because while it might be ok today, in the future the packages might end up with a function having the same name, and your users will get a warning every time your package is loaded.

Reading that I'd be tempted to either do the @import , or just bite the bullet of explicit pkg::function() calls. All the packages mentioned are already in the description file as far as I can see.

Interested to know your thoughts @rmbielby?

Ah interesting, missed that. How about @importFrom as another option? @chfoster's code for the disconnect screen doesn't work because the functions were neither imported or explicitly stated. So I guess we pick one and go with it. Or could leave to preference of whoever's coding a given function. Can see if you've got a lot of different functions in a piece of code, then @imporFrom could get a bit cumbersome, but same could be said for explicitly giving he package each time if you're using just one function a lot. So maybe it's just contextual (assuming both work functionally).

[cjrace]

Interesting - happy to help mop this up but not sure if blank importing whole packages for each function is the best way - https://roxygen2.r-lib.org/articles/namespace.html.

It is possible, but not generally recommended to import all functions from a package with @import package. This is risky if you import functions from more than one package, because while it might be ok today, in the future the packages might end up with a function having the same name, and your users will get a warning every time your package is loaded.

Reading that I'd be tempted to either do the @import , or just bite the bullet of explicit pkg::function() calls. All the packages mentioned are already in the description file as far as I can see.
Interested to know your thoughts @rmbielby?

Ah interesting, missed that. How about @importFrom as another option? @chfoster's code for the disconnect screen doesn't work because the functions were neither imported or explicitly stated. So I guess we pick one and go with it. Or could leave to preference of whoever's coding a given function. Can see if you've got a lot of different functions in a piece of code, then @imporFrom could get a bit cumbersome, but same could be said for explicitly giving he package each time if you're using just one function a lot. So maybe it's just contextual (assuming both work functionally).

I think in that case I'd recommend erring on the side of package::function() for all non-base R that's used in our code, as that's what I've already done in this PR. Can then leave it up to future developers of specific functions to by exception using a specific import (like @importFrom shiny tags) where it saves a lot of code.

Assuming we're happy with that, I'll add a note to the contributing section in the readme and then no further changes needed on this branch?

[rmbielby]

Yeah, think that makes sense. @chfoster's branch can illustrate the @importFrom option as she's using tags. Although at the moment, tags is sourced direct from htmltools, rather than via shiny (doesn't look like it makes much difference which is used as shiny$tags just acts as a wrapper for htmltools$tags, so either way htmltools gets loaded as a dependency somewhere on the way).

[rmbielby]

So just the library() lines still to remove right? Those don't do anything from within a package, because everything needs to be either explicit or listed under imported for the functions to be visible within a given function in the package?

[cjrace]

So just the library() lines still to remove right? Those don't do anything from within a package, because everything needs to be either explicit or listed under imported for the functions to be visible within a given function in the package?

Ah yes, forgot that - will push that now!