Closed nevrome closed 6 years ago
Merging #3 into master will increase coverage by
14.63%
. The diff coverage is48.18%
.
@@ Coverage Diff @@
## master #3 +/- ##
===========================================
+ Coverage 29.33% 43.97% +14.63%
===========================================
Files 5 6 +1
Lines 75 141 +66
===========================================
+ Hits 22 62 +40
- Misses 53 79 +26
Impacted Files | Coverage Δ | |
---|---|---|
R/zzz.R | 0% <ø> (ø) |
:arrow_up: |
R/c_test.R | 0% <ø> (ø) |
:arrow_up: |
R/utils.R | 41.07% <41.07%> (ø) |
|
R/doxygen_tools.R | 47.88% <55.55%> (+20.01%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 77f5c6f...32ed70d. Read the comment docs.
OK. Another option is to have doxy_vignette()
copy the doxygen doc to a subfolder of inst/doc
only if necessary. I still prefer it living in inst/doc/doxygen
though.
Also, I wonder if you'd consider a couple of changes to doxy()
:
First of all, it only works if you run it from the root folder of the package, as doxy_init
is setting INPUT
and OUTPUT_DIRECTORY
relative to there. Should at least provide a rootFolder
argument...
The doxygen
and roxygen
arguments both seem redundant. First, why would you ever want to set doxygen = FALSE
? :) Second, roxygen = TRUE
is not really rdoxygen's "job", in my view. Besides, it's just calling devtools::document()
, and is this really so annoying to type compared to the above?
Not trying to be nit-picky here, but yours is the only R package with Doxygen support. I think with something at e.g., R Bloggers or Rcpp Gallery you could get a lot of R/C++ users to work with your package.
Did you see my PR here?
You're right: The arguments doxygen
and roxygen
can be removed.
An article for the Rcpp Gallery is a great idea! I never considered this important enough to be honest.
Hi @nevrome, apologies for the slow response.
Thanks for the PR and I'm very glad you're open to some design suggestions. I have a couple more for you here. I very much look forward to your feedback on these thoughts as there are probably all sorts of unintended consequences I've overlooked. Once you make the final decisions on the design, I'm very happy to help out with implementation however I can.
As for the possibility of an Rcpp Gallery article, let me quickly gauge the interest first...
I guess you wrote this with the Rcpp-Gallery already in mind? Great!
Please excuse that it took me so long to answer. I'm currently finishing my master thesis (3 more weeks to go...).
Hi @nevrome, sorry for the slow response -- like you, I also have a day job :)
Thanks for the insightful feedback! (See replies above.) I think we've got most of the design issues ironed out.
Shall I get started on a PR implementing some of what we discussed above? I happen to be working on an R/C++ package using Doxgen extensively, so tinkering with rdoxygen would be a natural part of the workflow...
That sounds great! I suggest you just continue the development on the master branch of your fork. This PR can stay open to keep track of the work. Please consider merging my PR to your master before you continue. And don't forget to add yourself as author in the DESCRIPTION.
I'll chime in a few weeks. This overhaul could also be used to create a better testing infrastructure: The package should be a minimal example implementation by itself.
Deadline suggestion: At the end of October rdoxygen 2.0 should be ready, released and presented in a Rcpp Gallery post.
Sounds great indeed! I'll try to make frequent push/commits, in case you spot something off before it gets too deep :)
OK, here is a prototype for the new interface to doxy_init
, doxy_edit
, and doxy
. It seems to work fine on my project and the package's manual_tests
(which I took the liberty of converting to testthat
, see NOTE below).
For now the functions need to be run separately (i.e., can't do doxy(options = c(TAG = "VALUE"))
to run all three at once). Also, I followed your argument-naming conventions, but perhaps you'd like to rethink these before v2 -- as the package won't be backward-compatible anyways it seems like the right time to do so :)
So how about you make a PR to the "Arguments to Exported Functions" in rdoxygen-design.md
and we can take it from there?
(NOTE: Since CRAN tests won't have access to Doxygen, to avoid test failures I used the testthat::skip_on_cran()
option. This works fine if you test from within the package source directory using devtools::test()
, but you need to set Sys.setenv("NOT_CRAN", "true")
to test the installed version using testthat::test_package("rdoxygen")
.)
Merged now - more convenient to finish it on the master branch. @mlysy: I invited you as a collaborator.
See discussion in #2.
@mlysy. Thanks for the great work so far! I prefer to have an open pull request -- it makes the review and fine tuning process much more easy. And I also don't mind if this is still work in progress.