Skip to content

Conversation

hadley
Copy link
Member

@hadley hadley commented Dec 9, 2014

I've also disabled the examples because they cause R CMD check to fail

@yihui
Copy link
Contributor

yihui commented Dec 9, 2014

These are pretty much cosmetic changes, and I tend to put this PR off for now. If I will be the major maintainer of this package, I'll be likely to continue using Rd2roxygen instead of the standard roxygen2 process for a number of reasons:

  1. I'm often lazy with the code format of the examples (spaces, indentation, ...), and Rd2roxygen uses formatR to reformat the examples code by default (this can be turned off if not desired). When reformatting the code, it also tries to make all lines stay within 90 chars to avoid the R CMD check complaint;
  2. I added the function Rd2roxygen:::importRd() recently to deal with the tedious work of re-documenting objects that are essentially imported from other packages and exported in this package, e.g. %>%. Basically, it generates leaflet-imports.Rd automatically so we do not need to document any such objects manually. It is probably better for this functionality to be in roxygen2, but I have not got time to prepare a PR, nor do I know if roxygen2 wants it at all. This is what I originally wished in promptImport() in R 3.1.1 shiny#540. This problem should have been solved in base R, but they did not give a perfect solution (other than supporting \docType{import});
  3. I often want to rebuild the package vignette when I rebuild the package, which is not what RStudio currently does from Build & Reload.

Of course, if I'm not going to be the major maintainer, I can follow whatever the maintainer wants me to use :) For now, I tend not to discuss too much about personal styles.

@hadley
Copy link
Member Author

hadley commented Dec 9, 2014

I think these are all valid concerns (apart from 1 but we'll have to agree to disagree about the importance of code punctuation 😉). They are all issues that should be pushed upstream, and it's fine that you currently don't have the time to do that. (I know JJ is thinking about 3, and I'd be interested to think about a roxgen2 approach to solving 2.) But I think for an "RStudio" package, it's best to stick with standards, relying as much on possible on CRAN packages, so that anyone can pick it up and just use it.

@yihui
Copy link
Contributor

yihui commented Dec 9, 2014

We are talking about developers, who should be a tiny fraction of all users. General users can certainly pick it up and use it. Developers who want to contribute may or may not have trouble (we can put the instructions in CONTRIBUTING.md if necessary). I'll be happy to change my preferences when we run into real trouble. For now, I guess the priority is not here.

@jcheng5 jcheng5 closed this Dec 9, 2014
@yihui yihui deleted the roxygen2 branch January 9, 2015 22:44
jcheng5 pushed a commit that referenced this pull request Sep 6, 2016
jcheng5 pushed a commit that referenced this pull request Sep 12, 2016
* Fixes #7

* Deleting Magnifying Glass plugin fixes #252

* forgot to delete the R binding
bhaskarvk added a commit that referenced this pull request Sep 17, 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.

3 participants