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

Adding installIfNew.r example #59

Merged
merged 2 commits into from May 12, 2018

Conversation

Projects
None yet
2 participants
@1beb
Contributor

1beb commented May 7, 2018

Take a look, let me know what you think. I created a new file. It is essentially a copy of install2.r with some support for looking at installed.packages(). I added a warning when no packages will be installed but I'm not actually sure how that will be handled on passthrough.

}
}
pkgs <- setdiff(pkgs, installed.packages()[,1])

This comment has been minimized.

@1beb

1beb May 7, 2018

Contributor

Non trivial changes are here to L70-L77

@1beb

1beb May 7, 2018

Contributor

Non trivial changes are here to L70-L77

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 7, 2018

Owner

Let me take a closer peek at it in the next few days.

First comment, confirmed from running after glancing at the code: warning() is noise. Maybe just message() or even cat() ?

edd@rob:~/git/littler/inst/examples(pr59/install_if_new)$ ./installIfNew.R RcppGSL
Warning message:
In install_packages2(pkgs = f, lib = lib, repos = if (isMatchingFile(f)) NULL else rep,  :
  All specified packages are already installed.
edd@rob:~/git/littler/inst/examples(pr59/install_if_new)$ 

We may want to simplify and remove pieces from install2.r we do not need, like the -e support. I forget what that was motivated by, once.

Owner

eddelbuettel commented May 7, 2018

Let me take a closer peek at it in the next few days.

First comment, confirmed from running after glancing at the code: warning() is noise. Maybe just message() or even cat() ?

edd@rob:~/git/littler/inst/examples(pr59/install_if_new)$ ./installIfNew.R RcppGSL
Warning message:
In install_packages2(pkgs = f, lib = lib, repos = if (isMatchingFile(f)) NULL else rep,  :
  All specified packages are already installed.
edd@rob:~/git/littler/inst/examples(pr59/install_if_new)$ 

We may want to simplify and remove pieces from install2.r we do not need, like the -e support. I forget what that was motivated by, once.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 7, 2018

Owner

It is a pretty small change to install2.r. Do you think it could / should just be an additional toggle there?

Owner

eddelbuettel commented May 7, 2018

It is a pretty small change to install2.r. Do you think it could / should just be an additional toggle there?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 9, 2018

Owner

@1beb Any follow-up comment? As small as it is, should it be over in install2.r ? Default behaviour as is, if a (new) flag (say, --skip-installed or something) is set it does what you added.

Owner

eddelbuettel commented May 9, 2018

@1beb Any follow-up comment? As small as it is, should it be over in install2.r ? Default behaviour as is, if a (new) flag (say, --skip-installed or something) is set it does what you added.

@1beb

This comment has been minimized.

Show comment
Hide comment
@1beb

1beb May 9, 2018

Contributor
Contributor

1beb commented May 9, 2018

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel May 12, 2018

Owner

In order to get on with life I am going to merge this, and then delete the new file, carrying the diff over to install2.r under an appropriate flag.

Owner

eddelbuettel commented May 12, 2018

In order to get on with life I am going to merge this, and then delete the new file, carrying the diff over to install2.r under an appropriate flag.

@eddelbuettel eddelbuettel merged commit ec058c7 into eddelbuettel:master May 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@1beb

This comment has been minimized.

Show comment
Hide comment
@1beb

1beb May 13, 2018

Contributor
Contributor

1beb commented May 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment