Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update_collate removes existing Collate field whenever there are no @include directives #302

Closed
Geoff99 opened this issue Nov 12, 2014 · 1 comment

Comments

@Geoff99
Copy link
Contributor

@Geoff99 Geoff99 commented Nov 12, 2014

This issue manifested itself while I was trying to use devtools on a package with S4 classes defined in different files. To get devtools::load_all to work on the package, it needed to know the collate order, which I specified by using @include directives. But I needed to run roxygen2::update_collate to have those @include directives processed and placed into the Collate entry in the DESCRIPTION file, before devtools::load_all would succeed.

While trying to add a call to roxygen2::update_collate into (a forked development copy of) load_all, it emerged that some of the testthats in devtools had become unhappy. I tracked it down to the disappearance of Collate entries in the DESCRIPTION files of templates for testthat. The templates in question do not have @include directives, but did have manually entered Collate fields. My modified copy of load_all with a call to roxygen2::update_collate that had been called while running the devtools tests, wiped out the Collate entries that previously were there.

@hadley advised that this was probably a bug in update_collate - hence this issue.

See devtools/#623 for a minimal example -> \link{https://github.com/hadley/devtools/issues/623}

The sessionInfo from the test case demonstrating this behaviour was

> sessionInfo()
R version 3.1.1 (2014-07-10)
Platform: x86_64-w64-mingw32/x64 (64-bit)

locale:
[1] LC_COLLATE=English_Australia.1252  LC_CTYPE=English_Australia.1252   
[3] LC_MONETARY=English_Australia.1252 LC_NUMERIC=C                      
[5] LC_TIME=English_Australia.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] roxygen2_4.0.2

loaded via a namespace (and not attached):
[1] Rcpp_0.11.2   stringr_0.6.2 tools_3.1.1 

PS It's not as simple as not changing the collate entry if one already exists:

  1. If the user is using @include directives, there might be an outdated Collate field that ought to be refreshed.
  2. But if the user is entering the Collate field manually (which you might be able to guess if there are no @include directives found in the package code) then you should leave it alone.
@Geoff99
Copy link
Contributor Author

@Geoff99 Geoff99 commented Nov 13, 2014

Hi @hadley

I think I know why roxygen2::update_collate is overwriting the Collate field in the DESCRIPTION file, rather than doing nothing, when no @includes are found in the package code. It needs the insertion of a return() call if generate_collate returns NULL to make update_collate truly do nothing in that scenario. The details follow

Details of possible solution for this issue

The code for update_collate currently reads

update_collate <- function(base_path) { 
   collate <- generate_collate(file.path(base_path, "R")) 
   if (!is.null(collate)) { 
     collate <- paste0("'", collate, "'", collapse = " ") 
   } 


   desc_path <- file.path(base_path, "DESCRIPTION") 
   old <- read.description(desc_path) 


   new <- old 
   new$Collate <- collate 
   write.description(new, desc_path) 


   if (!identical(old, read.description(desc_path))) { 
     cat('Updating collate directive in ', desc_path, "\n") 
   } 
 } 

If no #``@includes are found by generate_collate then collate is NULL, in which case the line new$Collate <- collate
removes the Collate entry from the new list, and the following line
write.description(new, desc_path)
writes a new DESCRIPTION file without a Collate field.

To make nothing happen (including not over writing an existing Collate field) when there are no @includes present, there needs to be an else return() added to the end of the
if (!is.null(collate)) { etc }
segment of the function, ie it needs to read
if (!is.null(collate)) { etc } else return()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant