Notes from R CMD check

[ellessenne]

I get the following notes that could be easily fixed when running R CMD check on the package (on a macOS 10.14.6 machine running R 3.6.1):

* checking top-level files ... NOTE
Non-standard files/directories found at top level:
  ‘paper’ ‘test’

The 'paper' folder could be added to the .Rbuildignore file (a list of files that are ignored when building and testing the package), while the testing suite could be refactored to use the testthat package.

There are also a lot of no visible binding for global variable and no visible global function definition for notes, that should be fixed.

[seabbs]

A few more comments.

Global data frame calls + undocumented data.

I see calls to data within functions - loading data into the global environment like this isn't best practise (see ?data). Looking at improv_adjust.R I see no mention in the documentation of this external data frame. There are also multiple undocumented data frames.

Solution

As Adjust_table is built into the package you should be able to load it using something like

adjustment <- ANCRTAdjust::Adjust_table

I would like to see this changed, documentation for this data frame (example of this), documentation in the function mentioning that you are using this data frame and potentially a user argument to provide an override data frame. (so if this is NULL use the package built in)

Same for impcov_adjust_simple.R

No visible bindings

I see these a lot when using dplyr and ggplot2 in packages. My usual approach (although it may not be the best one) is to NULL out the variables at the beginning of the function call.

Example

global_var <- NULL; 

ggplot(data, aes(x = global_var)

Do you have a better solution @ellessenne?

For this #openjournals/joss-reviews/issues/1740 review

[ellessenne]

+1 for documenting the datasets, and I agree with the proposed solution mentioned above.

Regarding no visible bindings, I generally deal with this by using the utils::globalVariables function (e.g. here), but my understanding is that @seabbs's approach works just as well.

[seabbs]

I'd go with @ellessenne solution for visible bindings - much nicer than mine (I will be stealing it for my packages in the future).

[m-maheu-giroux]

Thanks for your suggestions. These changes have now been implemented

[seabbs]

Great - thanks.

Waiting on @ellessenne to review before closing.

[ellessenne]

I am happy with the changes, please feel free to close this issue!