Closed cjrace closed 6 months ago
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.
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
Interested to know your thoughts @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).
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?
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).
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?
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!
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
support_panel()
, more could be donesupport_panel()
, these are not exhaustiveAdditional information
Have raised issues #20, #21, #22 , #23, #24, #26, #27 off the back of this. To be addressed in separate PRs.