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

simple pkgdown check #393

Merged
merged 20 commits into from
Nov 15, 2022
Merged

Conversation

gravesti
Copy link
Contributor

@gravesti gravesti commented May 5, 2022

First attempt to implement quick checks for {pkgdown}. Fixes #378

@lorenzwalthert lorenzwalthert marked this pull request as ready for review May 7, 2022 17:02
@lorenzwalthert
Copy link
Owner

@gravesti thanks for your work. This works now in principle. Only practical problem are the heavly dependencies, in particular in a CI setting. So, it may work best if you use {pkgdown} already locally and have all the system dependencies. {pkgdown} has many dependencies and system dependencies that are required to make loading the package not error, but not required for our functionality...

Probably, these checks would just be skipped in CI then.

Coming from the problem use case: {pkgdown} builds are only triggered on default branches, so even if a PR succeeds, we don't know if the pkgdown build will succeed. Hence, instead (or as a complement) of a pre-commit hook, the GitHub Action for pkgdown could also build the page and not deploy it. Should I file an issue in r-lib/actions for this?

@lorenzwalthert
Copy link
Owner

I thought about it again and I think we should merge the hook, but use system as a language, r. Hence, we'd fallback on the system installation of {pkgdown} and all dev dependencies of a package, which will anyways be installed if you use the hook. In CI setting, using the hook makes no sense. Also, the lastet version of the {pkgdown} action in r-lib/actions already builds PRs as if the were merged in main, so I guess that is also an improvement.

@gravesti
Copy link
Contributor Author

I note that in the mean time pkgdown implemented a check_pkgdown() which could be used within the hook

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Nov 14, 2022

Thanks @gravesti. I spend at least 5h working on this PR (and some related issues that came up due to it) over the last 3 days. -.- I see that you mentioned that you opened an issue in {pkgdown}, can you please link it next time? Would have saved me a lot of time I think.

@lorenzwalthert lorenzwalthert merged commit 59981a7 into lorenzwalthert:main Nov 15, 2022
@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Nov 15, 2022

@gravesti this will be available when there is a new release (i.e. new tag on main). Or if you use a hash pointing to HEAD (e.g. 59981a7) instead of a git tag.

@lorenzwalthert lorenzwalthert mentioned this pull request Dec 6, 2022
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.

New hook to check contents of _pkgdown.yaml contains all topics
2 participants