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

Improved install phantomjs #82

Merged
merged 15 commits into from
May 24, 2019
Merged

Improved install phantomjs #82

merged 15 commits into from
May 24, 2019

Conversation

coatless
Copy link
Contributor

@coatless coatless commented May 21, 2019

This PR implements the PhantomJS installation check as described in #25 by:

  1. Adding an is_phantomjs_installed() function that returns TRUE/FALSE if PhantomJS is installed or not.
  2. Modifying the parameter of install_phantomjs() to include force, which controls whether PhantomJS will be installed or not if it already is present on the user's computer.

Copy link
Owner

@wch wch left a comment

Choose a reason for hiding this comment

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

It would be better if install_phantomjs checked not just for the presence of phantomjs, but also the version. If phantomjs is not present or if it is an older version, then install it. (Using force=TRUE still always install it.)

NEWS.md Outdated Show resolved Hide resolved
@coatless
Copy link
Contributor Author

@wch thanks. I'll add in a version check.

@coatless
Copy link
Contributor Author

@wch I've added in the version check. To do so, I've created three internal functions:

  1. phantomjs_cmd_result()
    • Passes its arguments to the phantom_run() function.
    • Captures and stores phantom_run()'s STDOUT information instead of only displaying it on console.
  2. phantomjs_version()
    • Retrieves the phantomjs version via path/to/bin/phantom --version obtained under phantomjs_cmd_result()
  3. is_phantomjs_version_latest()
    • Compares the installed version with requested version and determines if it is out of date.

Copy link
Owner

@wch wch left a comment

Choose a reason for hiding this comment

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

A few minor things, but otherwise it looks good.

R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
@coatless
Copy link
Contributor Author

@wch ready for round 3.

R/utils.R Outdated

if (is_phantomjs_version_latest(version) && !force) {
message('It seems that the installed version of `phantomjs` is the latest. ',
'To reinstall the version presently installed, use `force = TRUE`.')
Copy link
Owner

@wch wch May 24, 2019

Choose a reason for hiding this comment

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

Slight correction: this won't necessarily install the presently-installed version; it will install the version specified by the version parameter. Also, getting here doesn't necessarily mean that the latest version is installed; it just means that the version is >= to version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the message to reflect this.

@coatless
Copy link
Contributor Author

@wch let's try round 4?

@wch wch merged commit 35ec371 into wch:master May 24, 2019
@wch
Copy link
Owner

wch commented May 24, 2019

Thanks!

@coatless coatless deleted the improved-install-phantomjs branch May 24, 2019 20:10
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.

2 participants