Rely on roxygen2 more

[hadley]

Much of the previous code is no longer necessary as roxygen2 handles it internally. Once we can move roxygen2 back into imports, we can add a version dependency there and then delete the old code.

[jimhester]

Couldn't we just depend on roxygen 6.1.0 now that it is on CRAN, even with it still in Suggests? We already depend on roxygen 5.0.0 there. IIRC check_suggested() (called above) will throw an error for older versions if a version dependency is in the DESCRIPTION.

[hadley]

My memory is that the version requirements of suggested packages aren't enforced, but I might be wrong.

[gaborcsardi]

My memory is that the version requirements of suggested packages aren't enforced, but I might be wrong.

WRE has:

Version requirements can be specified but should be checked by the code which uses the package.

Kind of makes sense for a suggested package.

[jimhester]

Right, but we do check this explicitly with pkgload::check_suggested() earlier in this function, although you get a warning rather than an error. (I artificially bumped the version in the DESCRIPTION as a test).

> check_suggested("roxygen2")
Warning in check_dep_version(package, version) :
  Need roxygen2 >= 7.0.0 but loaded version is 6.1.0

[jimhester]

We can remove this conditional, the only way you could be in this block is by being less than "6.1.0"

[hadley]

Doh

[hadley]

Hmm, looks like it should be an error? I'm in favour of just bumping if you're ok with it.

[jimhester]

Yeah just bumping sounds good to me.

[hadley]

Appveyor failure seems unrelated; I think this is good to merge.

[jennybc]

I am finding that I need this PR in order for devtools::document() to do the right thing with roxygen2 v6.1.0. Specifically, without this PR, re-document()ing after changing a function signature does not, in fact, cause the corresponding change in the .Rd. The signatures appear to persistently derive from the installed version, as opposed to current source.

Here's what happens if you create a new package with usethis::create_package() and add an exported, roxygen-documented function (but you never install the package):

==> devtools::document(roclets=c('rd', 'collate', 'namespace'))

Updating aabb documentation
First time using roxygen2. Upgrading automatically...
Writing NAMESPACE
Error in as.environment(where) : using 'as.environment(NULL)' is defunct
Calls: suppressPackageStartupMessages ... block_find_object -> object_from_call -> parser -> exists
Execution halted

Exited with status 1.

A different consequence of this PR that might be undesirable is that the helpers = FALSE behaviour (in the sense of load_all(pkg$path, helpers = FALSE)) has been lost. Is that worth taking a stand on?

[jimhester]

Thanks!

[jennybc]

I think this is still worth pondering but I guess it's a roxygen2 matter now (?):

A different consequence of this PR that might be undesirable is that the helpers = FALSE behaviour (in the sense of load_all(pkg$path, helpers = FALSE)) has been lost. Is that worth taking a stand on?

[hadley]

Would you mind filing a roxygen2 bug so I don't forget?

[jimhester]

Maybe we should be calling pkgload::load_all(helpers = FALSE) in roxygen2 and maybe export_all = FALSE to more closely mimic a normal package namespace? I am not sure what roxygen2 assumes in regards to exports.

[jimhester]

Also potentially recompile = FALSE, which would be a way to fix r-lib/roxygen2#771, but maybe would cause other issues if the dll was not built.

[hadley]

Yeah, that seems like a good idea.