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

pass pkgdown.internet option in build_site_external #877

Merged
merged 3 commits into from Nov 10, 2018

Conversation

Projects
None yet
3 participants
@cderv
Copy link
Contributor

cderv commented Nov 8, 2018

This fixes #861 by passing the option pkgdown.internet that could be set by the user to build_site_external call to callr::r(). It completes #774.

This now works:

options(pkgdown.internet = FALSE)
pkgdown::build_site()

I did not look yet if I should add a test for build_site_external.

@jayhesselberth

This comment has been minimized.

Copy link
Collaborator

jayhesselberth commented Nov 9, 2018

Looks good to me. Thanks!

@cderv

This comment has been minimized.

Copy link
Contributor Author

cderv commented Nov 9, 2018

About the error in Travis, it does not seem related to this PR.

About NEWS.md, I did not added a bullet as there is already one about the option pkgdown.internet. Should we update NEWS with the PR number only ?

Users with limited internet connectivity can explicitly disable pkgdown CRAN checks by setting options(pkgdown.internet = FALSE) prior to running build_site() (#774, #877)

About test, I looked into it but I did not found where to test and how because it is really specific. If you want some test, please advice.

Finally, I wanted to add further documentation about internet connectivity requirement to explain that if the package for which pkgdown is used requires internet connection for example or vignette, this has nothing to do with pkdown.internet option and it won't works offline.
I did not find where to put that. There is an internet section in build_home but not build_site.
Would you like me to add this ? In a new section in build_home or addition to the existing one ? In the documentation of build_reference and build_articles ?

Thanks.

Show resolved Hide resolved R/build.r Outdated
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Nov 9, 2018

Yes, that's the right strategy for the news bullet.

For the docs, maybe move the existing section from build_home() to build_site() and expand? That might be better in a separate PR.

Thanks!

cderv added some commits Nov 9, 2018

@jayhesselberth jayhesselberth merged commit 7dbcc59 into r-lib:master Nov 10, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cderv cderv deleted the cderv:fix-internet-external branch Nov 10, 2018

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