devtools::load_all() may fail when S4 class definitions are in different files

[Geoff99]

Hi
This is similar to something I tried to raise in #550, but in the previous issue I confused two different aspects of my problem when I try to use the devtools package development sequence, so I am trying again, just concentrating on explaining the aspect that causes devtools to fail for me.

If I create a simple example project and add two files in the R directory as follows
a.r is

#' @include b.r
Class2 <- setClass('Class2',contains='Class1')

and b.r is

Class1 <- setClass('Class1')

then when I try to run load_all() on this package I get the following error messages

> library(devtools)
> load_all()
Loading loadallExample
Error in reconcilePropertiesAndPrototype(name, slots, prototype, superClasses,  (from a.r#2) : 
  no definition was found for superclass “Class1” in the specification of class “Class2”
> 

The problem arises because although I have the @include directive in my file a.r, it has not yet been processed, so load_all() attempts to read the code from the files in alphabetical order.

I still get an error if I try and run devtools::document('.'), as seen here

> document('.')
Updating loadallExample documentation
Loading loadallExample
Error in reconcilePropertiesAndPrototype(name, slots, prototype, superClasses,  (from a.r#2) : 
  no definition was found for superclass “Class1” in the specification of class “Class2”
> 

And because of the error while loading, the document('.') does not finish, and does not update the collate entry in the package DESCRIPTION file.

A solution is to run roxygen2::update_collate() before trying to load_all() or document() my package

> roxygen2::update_collate('.')
Updating collate directive in  ./DESCRIPTION 
> load_all()
Loading loadallExample
> 

The updated collate entry in the DESCRIPTION file created by roxygen2::update_collate tells load_all() (and all the other devtools machinery) the correct order to process the files in.

I can avoid this problem myself now I know what causes it, but as an absolute novice package creator it really confused me for ages. In my real life example the alphabetic order of the file names was not so obvious, and when I tried to pin the error down by commenting out bits of code, it enabled devtools::document to run successfully. Then when I removed the commenting out, the original code would behave nicely, making the problem appear only intermittently and very frustratingly. It would be nice if some fix of the type I have illustrated could be included in the official devtools.

Thanks

Geoff

PS I'm still pretty much a novice package creator (waiting keenly for the book to be finished and published), just a little more informed about this particular problem.

PPS Because roxygen2::update_collate emits a message when it runs, I think simply adding a call to roxygen2::update_collate into the code for load_all may cause some of the testthat tests on devtools to fail (because they are not expecting that particular message to be there)

PPPS I have tried to write a testhat for devtools to check that the collate order is correct and up to date. The testthat code is

test_that("Collate sequence is uptodate, and @includes have been recognised", {
  # Make a temporary clean copy of the test package, in order to
  #   avoid (unwanted here but usually beneficial) side_effects
  #   namely that devtools::load_all may update the collate order in the
  #   DESCRIPTION file
  clean_testCollateOrder <- file.path(tempdir(),'testCollateOrder')
  dir.create(clean_testCollateOrder)
  test_pkg_files <- c('testCollateOrder/DESCRIPTION',
                      'testCollateOrder/NAMESPACE',
                      'testCollateOrder/R')
  file.copy(test_pkg_files,clean_testCollateOrder,recursive=TRUE)

  expect_message(load_all(clean_testCollateOrder), "Loading testCollateOrder")

  expect_equal(a, 1) #even though b.r set it to 2

  unload(clean_testCollateOrder)
  unlink(clean_testCollateOrder,recursive=TRUE)
})  

It requires a template project (which I have called testCollateOrder) with a plain vanilla DESCRIPTION file

Package: testCollateOrder
Title: Confirm that the collate order is uptodate
Version: 0.1
Authors@R: "Geoff Lee<[email protected]> [aut, cre]"
Description: load_all fails with an error when S4 classes with inheritance
  are read in the wrong order.  This package checks that all @includes
  have been recognised and processed by roxygen2,
  so that the collate order in the DESCRIPTION file is uptodate
Depends: R (>= 3.1.1)
License: What license is it under?
LazyData: true

The key feature is that there is no collate entry in the file yet.

The two files in the R directory are
a.r

#' @include b.r
a <- 1

and b.r

a <- 2

The test will succeed if the @include directive has been recognised so that a.r is read after b.r

[Geoff99]

PPPPS The sessionInfo for the examples 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] loadallExample_0.1  devtools_1.6.0.9000

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

[wch]

Have you tried updating to the most recent roxygen2?

[Geoff99]

I have just now updated from roxygen2 4.0.1 to 4.0.2 (the CRAN version of roxygen2, not the very latest github version) and I still get the same problem (as long as I remember to clean out the DESCRIPTION file so it has no collate entry, so that the example test is valid - once I've run roxygen2::update_collate() once, everything is of course fixed in the DESCRIPTION file my example )

> library(devtools)
> load_all()
Loading loadallExample
Error in reconcilePropertiesAndPrototype(name, slots, prototype, superClasses,  (from a.r#2) : 
  no definition was found for superclass “Class1” in the specification of class “Class2”
> library(roxygen2)
> 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      devtools_1.6.0.9000

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

I didn't think of checking the latest version of roxygen2 at first (but I should have done) because it looks like the failure happens before roxygen2 is invoked - apologies for the oversight.

Geoff

[Geoff99]

Hi Winston

I hadn’t at the time I posted the issue, but I did as soon as I saw your question, and the short answer is, using the latest CRAN version of roxygen2 doesn’t fix the issue (I think the error occurs before roxygen2 is invoked).

Geoff

PS I’ve posted the details on github – I’m still getting used to what github does with notifications and watching etc, so apologies in advance for the duplication if you already saw my response there

From: Winston Chang [mailto:[email protected]]
Sent: Tuesday, 21 October 2014 2:16 AM
To: hadley/devtools
Cc: Geoff99
Subject: Re: [devtools] devtools::load_all() may fail when S4 class definitions are in different files (#623)

Have you tried updating to the most recent roxygen2?

—
Reply to this email directly or view it on GitHub #623 (comment) . https://github.com/notifications/beacon/8340230__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcyOTQzNzM1MywiZGF0YSI6eyJpZCI6NDYyNTYyODl9fQ==--9b2462337da10c8fa9dbc3715c3c78d8a42c1e36.gif

[hadley]

I think we should probably call roxygen2::update_collate in load_all() - if there are no @include tags, this should leave the description as is.

[Geoff99]

Thanks Hadley. That's what I thought after I'd read through the load.r code. It seems to be on the same plane as creating a default DESCRIPTION file if none exits.

#' @include seems philosophically a little bit unusual as a roxygen2 comment - it isn't pure documentation since it has a side effect (expressed via the collate entry in the DESCRIPTION file) on what the packaged code actually does. But it's very useful - much more natural to me than compiling the collate list manually and editing it into the DESCRIPTION file.

PS I couldn't quite figure out whether or not it was necessary to call as.package (possibly again) after the call to roxygen2::update_collate in order to reflect the updated collate field in the information stored about the pkg in devtools online memory

[Geoff99]

Hi @hadley
I've been working on this (very slowly, since I'm still learning the very basics of git, and happily working my way through my copy of the Advanced R book which arrived a couple of weeks ago).

I've hit a snag, which means I've got some more thinking to do. If you have any guidance to offer, it would be most appreciated.

Edited in some thoughts about options (on Mon 11 Nov 2014) - see below

It seems that roxygen2::update_collate overwrites the existing Collate field in the DESCRIPTION file, based on the @include directives that it finds in the files in the package directory.

That's good for my situation :-) (as I slowly build up the package I am working on, and tidy the functions into nice workable and maintainable code, I want the Collate entries to be changed to reflect my latest file structure.)

But if someone else has chosen to create their Collate entries by manually editing the DESCRIPTION file instead of using the @include roxygen2 directives, this will wipe out their entries, and make them unhappy :-(

It may need a bit more work to figure out which situation applies and hence to guess what the user actually wants to do.

Regards

Geoff

PS This has been a really good example for me to appreciate the value of the testthat approach to development. I figured this out only after I started to try and work out why some of the existing expectations in test-load-collate.r were failing after I added the call to update_collate in my forked branch of devtools.

Edited in - thoughts about possible approaches

I can think of three possible approaches, none feel really good. The last one (3b) seems the least worst to me.

Options

1. Mandate that if the collate order matters to your package and you want to use devtools then either:
   1a. You must use @include and cannot edit the DESCRIPTION file collate field by hand or
   1b. You cannot @include and must edit the DESCRIPTION file collate field by hand
2. Add a new argument to devtools::load_all which indicates whether or not you want the collate field to be generated from @include directives; make sure that all internal uses of load_all are aware of this, and pass the appropriate argument through
3. Do your best to guess what the package builder wants, as follows

'@includes` exist in package files	Guessed action
Yes	update_collate and overwrite any existing collate entries
No	leave collate entry (if it exists) unchanged, ie do not update_collate

3a. Find out about the existence of any @includes by mirroring parts of the roxygen2 code and / or calling unexported roxygen2 functions (bad :-( )
3b. Find out about the existence of any @includes by taking a safecopy of the DESCRIPTION file, running update_collate, then seeing if the new updated DESCRIPTION file has a non-null collate entry

Any comments and better suggestions much appreciated, Geoff

[hadley]

If there are no @include directives, roxygen shouldn't touch the collate. If it does, it's a bug.

[Geoff99]

I think there is a bug in the version of roxygen2::update_collate I am using then.

If I have a test package with the following DESCRIPTION file

Package: roxygenbugexample
Title: Checks Whether roxygen2::update_collate Removes Entries
Version: 0.0.0.9000
Authors@R: "First Last <[email protected]> [aut, cre]"
Description: Simple package with no @includes and a pre-existing Collate entry
Depends: R (>= 3.1.1)
License: What license is it under?
LazyData: true
Collate: b.r a.r

and then run roxygen2::update_collate() on it

> library(roxygen2)
> roxygen2::update_collate('.')
Updating collate directive in  ./DESCRIPTION 

the DESCRIPTION file becomes

Package: roxygenbugexample
Title: Checks Whether roxygen2::update_collate Removes Entries
Version: 0.0.0.9000
Authors@R: "First Last <[email protected]> [aut, cre]"
Description: Simple package with no @includes and a pre-existing Collate entry
Depends:
    R (>= 3.1.1)
License: What license is it under?
LazyData: true

i.e. the collate entry is removed rather than nothing happening (there are no @includes in a.r or b.r)
Here is the sessionInfo from my run.

> 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 

I'll file an issue for roxygen2 (after I check that I am using an up to date version of roxygen2)

[Geoff99]

Hi @hadley

Not sure if it is the correct protocol to comment on a closed issue but I'd like to reopen the issue / suggest an additional change.

The test_that (from pull request #649) load_all() recognises and processes the @include generated collate order is still failing, even though load_all now includes a call to roxygen2::update_collate.

It appears that as well as calling update_collate to create / refresh the Collate field in the DESCRIPTION file, devtools also needs to refresh the pkg$collate entry in the pkg list / structure so that the remainder of the load_all() code is aware that the collate order has been generated / refreshed and is (possibly) non-alphabetical.

The following extra line (after the commented explanation) does this

roxygen2::update_collate(pkg$path)
#Refresh the pkg structure with any updates to the Collate entry
#in the DESCRIPTION file
pkg <- as.package(pkg$path)

[hadley]

Can you please refile a pull request with that change and the tests?

[Geoff99]

Sure, I’ll be happy to.

Will be later today (my time) before I get chance to make sure I’ve got everything neat and tidy

Geoff

From: Hadley Wickham [mailto:[email protected]]
Sent: Thursday, 15 January 2015 8:22 AM
To: hadley/devtools
Cc: Geoff99
Subject: Re: [devtools] devtools::load_all() may fail when S4 class definitions are in different files (#623)

Can you please refile a pull request with that change and the tests?

—
Reply to this email directly or view it on GitHub #623 (comment) . https://github.com/notifications/beacon/AH9DBq19PCwxhux-wZnVr48lAZIsiUuHks5nhtWTgaJpZM4CwdCh.gif

[Geoff99]

Please see pull request #649

Happy to make any adjustments you want

Geoff

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/