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

This makes the scripts executable on a fresh R installation. #42

Merged
merged 2 commits into from
Jul 16, 2020

Conversation

lemire
Copy link
Collaborator

@lemire lemire commented Jul 16, 2020

This may not be a good idea, but for someone like myself who only use R from time-to-time, and often on a fresh R setup, having self-contained scripts that install their own dependencies is quality-of-life issue.

@lemire lemire requested a review from eddelbuettel July 16, 2020 14:31
@eddelbuettel
Copy link
Owner

About to go for a (belated) run and will clean the commit up a little later. These days the recommended idiom is

if (!requireNamespace("somepackage", quietly=TRUE) doSomethingHere()

It is a matter of taste if make scripts install their requirements. I used to do it a little more ~20 years ago, I now find it (mostly) redundant as we a) do work with packages anyway and b) packages take care of this.

It is definitely useful for optional packages having a Suggests: in DESCRIPTION.

Lastly, compulsory advertisement: You want probably want littler to give you r (at least on real computers, those infected by some Cupertino-based disease need to alias to lr or some such as the brain surgeons over there default to r being equal to R. Grrrrrrr.)

README.md Outdated
@@ -26,6 +26,10 @@ QCon](http://www.youtube.com/watch?v=wlvKAT7SZIQ) (voted best talk).
### Example

```r
if (!require("RcppSimdJson")) install.packages("RcppSimdJson")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On balance, no.

In a README.md conciseness trumps completeness. It is also the top-level README for the package. We will assume people can install it.


if (!require("microbenchmark")) install.packages("microbenchmark")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simply a stopifnot() which now has customizable errors.

Auto-installation is no panacea either as not every user will have a compiler.

if (!require("jsonlite")) install.packages("jsonlite")
if (!require("RcppSimdJson")) install.packages("RcppSimdJson")
if (!require("ndjson")) install.packages("ndjson")
if (!require("RJSONIO")) install.packages("RJSONIO")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes some sense here and I will clean that up later.

@lemire
Copy link
Collaborator Author

lemire commented Jul 16, 2020

This PR can be thrown away without hurting my feelings at all. ;-)

@eddelbuettel
Copy link
Owner

I added a small amount of spit and polish. Let me know what you think.

@lemire
Copy link
Collaborator Author

lemire commented Jul 16, 2020

I think that this is much better!!! I recommend merging this.

@eddelbuettel eddelbuettel merged commit c4f53c2 into eddelbuettel:master Jul 16, 2020
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