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

CRAN ready #1

Merged
merged 7 commits into from
Dec 23, 2016
Merged

CRAN ready #1

merged 7 commits into from
Dec 23, 2016

Conversation

fabian-s
Copy link

Summary of changes:

  • roxygenized package: automatic exports, imports, generation of Rd files.
  • got rid of all R CMD check warnings/errors
  • cleaned up code formatting
  • added README.md
  • add travis infrastructure for automatic R CMD check on every commit on 3 R versions (old release, current, devel). You'll need to adapt the link in the README.md to your own Travis CI account after merging.

See comments under TODOs in dlnm-package.R, l. 81 for what's still missing:

  • argument order of exphist, exphistint is different - why? can this be changed?
  • vignette Rnw-sources are missing
  • "fun"-string in onebasis, checkonebasis instead of handing over a proper function invites nasty scoping issues that seem to break examples in R CMD check, so I set those to ' \dontrun' . They still work fine in the console.

@gasparrini
Copy link
Owner

Thanks for your suggestions. I add later some comments on the other changes you suggest, and discuss here below why I am skeptical about 'roxygenizing' the Rd files and namespace, and I prefer keeping these as they are. Please let me know how you want to proceed.

  • The documentation of code (in R files) and functions (in Rd files) offer different levels of information for the users. In particular, the latter must be written for general users who simply want to understand the usage of functions, not the internal code;
  • The documentation in Rd files can refer to different parts of the code, and possibly can include calls to internal functions, which is difficult or cumbersome to document within R files;
  • Adding text for documentation meant for Rd files clutters the R files, which are less readable.
  • It is preferable to have direct control of the namespace (see also comments below), also considering that problems/conflicts are identified by Rcmd check;
  • The use of roxigen2 implies the use of additional syntax/steps and less control on the format/content of Rd files, without substantial advantages in terms of time/clarity.

Regarding the other changes:

  • I'm happy about the inclusion of Travis CI checks. I created my account there and will add the link in README.md;
  • I'm happy about the addition of the README.md file;
  • I agree with changing the arguments of exphist and exphistint;
  • I removed the Rnw files for the vignettes as they slowed down Rcmd check. I will put them back now;
  • I prefer to keep the formatting as it is, in particular the '#' signs and the (lack of) spacing in some functions. The former comes from pre-RStudio browsing of the functions, the latter makes the code shorter;
  • I am not sure about which changes you included in onebasis and checkcrossbasis to avoid warnings (and which warnings are in fact generated);
  • Which other error/warnings were generated? Are they those reported in the CRAN check, with some --no-stop-on-test-error messages (see https://cran.r-project.org/web/checks/check_results_dlnm.html)?

@fabian-s
Copy link
Author

fabian-s commented Dec 22, 2016

Not gonna argue about roxygen with you, I think it's a huge mistake to not do this but it's your package and your funeral. Re. the code formatting you'll have a really hard time to disentangle my style changes from the actually changed code... Sorry about that, but, I'm sorry to say, the style you used is quite idiosyncratic and it's hard to collaborate on code that's hard to read because it's not formatted in a standard way...

Which other error/warnings were generated? Are they those reported in the CRAN check, with some --no-stop-on-test-error messages

I don't remember -- does it matter? Main thing is at the package now R CMD checks without errors/warnings, right?

@gasparrini gasparrini merged commit 3744e9b into gasparrini:version227 Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants