search

rOpenGov / pxweb

R tools to access PX-WEB API
http://ropengov.github.io/pxweb
Other
69 stars 31 forks source link

Fix missing pages on gh deployment #248

Closed dieghernan closed 2 years ago

dieghernan commented 2 years ago

Fix #245

Issue was that some additional packages (devtools?) was added on the article. While this is not a problem when checking the package (articles are ignored, vignettes are not), but when running pkgdown all needed packages needs to be installed.

Packages needed on vignettes are declared on DESCRIPTION, while articles can have additional packages (I often do this), but then it is neccesary to ensure that the GH action also installs the packages needed on the articles.

Hope this make sense, regards

pitkant commented 2 years ago

Looks like we were working simultaneously on the same issue :D Your solution is generally speaking the correct way to declare needed packages, especially in the case of articles, but here it was about an actual vignette... remotes was also already listed in Suggests whereas devtools was not so to me the simplest solution was to change the vignette example to use remotes instead of devtools, since remotes is anyway the preferred way to install development versions from Github, at least according to some users.

What I found a bit puzzling was that pkgdown seemed to require devtools even if the code chunk in question was eval = FALSE, why is that?

dieghernan commented 2 years ago

What I found a bit puzzling was that pkgdown seemed to require devtools even if the code chunk in question was eval = FALSE, why is that?

Honestly I don’t know, it is weird indeed

MansMeg commented 2 years ago

This is great! Can you decide on which PR to merge and then close the other one? =)

dieghernan commented 2 years ago

I leverage here on @pitkant decision.

pitkant commented 2 years ago

I guess both could be merged: First this PR to pxweb/pitkant branch and my PR to pxweb/master branch (although that leaves any::devtools line redundant, I think)

MansMeg commented 2 years ago

Ok. Could you set that up so I can merge?

MansMeg commented 2 years ago

Sorry. Saw that now.