RPVote / eiCompare

Comparing ecological inference techniques
https://rpvote.github.io/voting-rights/
6 stars 7 forks source link

devtools::document() does not overwrite NAMESPACE #15

Closed aridf closed 4 years ago

aridf commented 4 years ago

Assigning @woodsp because he posted on Slack about roxygen2 a while back.

Per Hadley's textbook, the preferred method for writing documentation is via roxygen2 comments above functions. Here's an example from some code I am working on for this package:

#' Create a dataframe with NA values for race and candidate counts.
#'
#' For use to build toy test datasets
#' 
#' @param ncand The number of candidates to include
#' @param nrace The number of race/ethnicities to include
#' @param nrow The number of rows for the dataframe
#' 
#' @return A dataframe with columns for each candidate and race, all with NAs

empty_ei_df <- function(ncand = 3, nrace = 3, nrow = 10) {
     function content here
}

Running devtools::document() should add this function's .Rd file to the package and update the NAMESPACE to include information about this function. The latter is not happening. When I run devtools::document() I get the following warning:

Warning: The existing 'NAMESPACE' file was not generated by roxygen2, and will not be overwritten.

This seems like it will cause problems down the line. Perhaps we need to delete NAMESPACE and let devtools::document() rewrite it from scratch, but I'm not sure whether we will lose important information from doing this.

scottyhq commented 4 years ago

@aridf - i'd say it's okay to break things here, that's the beauty of forks! on your branch go ahead and delete the existing NAMESPACE file (you can always get it back since it is in the git history). And let's see how the auto-generated file compares

woodsp commented 4 years ago

I second Scott's vote to delete NAMESPACE and potential break eiCompare.

And if I understand the issue correctly, it will truly break the package, because eiCompare will no longer be able to directly call functions from other packages that are currently being imported into the namespace. Instead, those functions will require the :: operator everywhere in eiCompare (eg, as you've done for the document function in devtools). And the packages should be listed as dependencies in the DESCRIPTION file. If these aren't difficult improvements to make to eiCompare, they are what I understand to be current best practice.

scottyhq commented 4 years ago

running devtools::document() on the migrate-docs branch I'm getting the following error:

── Conflicts ──────────────────────────────────────────── eiCompare conflicts ──
✖ .Random.seed() masks eiCompare::.Random.seed()

@lorenc5 - how is eiCompare-internal.R used in practice? If I remove that file the error disappears.

lorenc5 commented 4 years ago

That I think is a file probably randomly created when you build the package. Honestly, I do not know much other than googling around a while back why that's even there. Just I can vaguely recall getting errors about it at some point when I uploaded to CRAN.

On Fri, Jul 24, 2020 at 11:59 AM Scott Henderson notifications@github.com wrote:

running devtools::document() on the migrate-docs branch I'm getting the following error:

── Conflicts ──────────────────────────────────────────── eiCompare conflicts ──

✖ .Random.seed() masks eiCompare::.Random.seed()

@lorenc5 https://github.com/lorenc5 - how is eiCompare-internal.R used in practice? If I remove that file the error disappears.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DSSG-eiCompare/eiCompare/issues/15#issuecomment-663661391, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGKPEZTBV46TFFEAYS42SDDR5HDX5ANCNFSM4O2V3XPA .

-- Loren Collingwood Associate Professor Department of Political Science University of California, Riverside collingwoodresearch.com

aridf commented 4 years ago

Closing this issue since we dealt with it by doc migration.