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

Fix use_readme_rmd() so that a pre-commit hook is registered if pkg uses git #41

Merged
merged 12 commits into from Dec 19, 2017

Conversation

Projects
None yet
3 participants
@PeteHaitch
Copy link
Contributor

PeteHaitch commented Aug 2, 2017

Following up on r-lib/devtools#1512

AFAICT, this small bug meant that the registration of the pre-commit hook was never triggered because the file path was not properly constructed.

I couldn't figure out how you are planning to test usethis, so I haven't ported the unit test from the original PR. Will do it if you can point me in the right direction.

R/readme.R Outdated
@@ -33,7 +33,8 @@ use_readme_rmd <- function(base_path = ".") {
)
use_build_ignore("^README-.*\\.png$", escape = FALSE, base_path = base_path)

if (uses_git(base_path) && !file.exists(base_path, ".git", "hooks", "pre-commit")) {
if (uses_git(base_path) &&
!file.exists(file.path(base_path, ".git", "hooks", "pre-commit"))) {

This comment has been minimized.

@hadley

hadley Aug 9, 2017

Member

Maybe it would be better to create a function for constructing this url? I presume use_git_hook also has to do the same computation. Then you can test that function

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 12, 2017

Are you still interested in finishing this off?

@PeteHaitch

This comment has been minimized.

Copy link
Contributor Author

PeteHaitch commented Oct 12, 2017

Yes, but have been swamped with other commitments. Will return to it tomorrow afternoon or next week.

R/git.R Outdated
stop("This project doesn't use git", call. = FALSE)
}

base_path <- git2r::discover_repository(base_path)

This comment has been minimized.

@PeteHaitch

PeteHaitch Oct 17, 2017

Author Contributor

git2r::discover_repository() includes the .git as the final part of the path it constructs

R/git.R Outdated
@@ -71,15 +71,15 @@ restart_rstudio <- function(message = NULL, base_path = ".") {
#' @family git helpers
#' @export
use_git_hook <- function(hook, script, base_path = ".") {
if (uses_git(base_path)) {
if (!uses_git(base_path)) {

This comment has been minimized.

@PeteHaitch

PeteHaitch Oct 17, 2017

Author Contributor

This check was backwards

R/readme.R Outdated
@@ -33,7 +33,7 @@ use_readme_rmd <- function(base_path = ".", open = TRUE) {
base_path = base_path
)

if (uses_git(base_path) && !file.exists(base_path, ".git", "hooks", "pre-commit")) {
if (uses_git(base_path)) {
use_git_hook(

This comment has been minimized.

@PeteHaitch

PeteHaitch Oct 17, 2017

Author Contributor

use_git_hook() warns if the file exists and is different from that being created

@PeteHaitch

This comment has been minimized.

Copy link
Contributor Author

PeteHaitch commented Oct 17, 2017

The failing build on R/3.1 is due to something in git2r, I think, but I don't understand it.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 17, 2017

Can you please skip that test on R 3.1? You can use testthat::skip_if_not(getRversion() > 3.1), and you'll need to include a brief comment about why (something about git2r)

@PeteHaitch

This comment has been minimized.

Copy link
Contributor Author

PeteHaitch commented Oct 17, 2017

Done. Pushed to https://github.com/PeteHaitch/usethis but it's not showing up here in the PR nor was a Travis build triggered. Have I done something wrong?

jennybc added some commits Dec 19, 2017

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Dec 19, 2017

I resolved the conflicts here and this PR works for me now.

@PeteHaitch If you will grant me permission to push into your PR, I can push an update for the tests as well, which should clear up the Travis failures. Then, if @hadley approves, we can merge this.

Here's how to grant me permission to add commits to your PR:
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@PeteHaitch

This comment has been minimized.

Copy link
Contributor Author

PeteHaitch commented Dec 19, 2017

Hi @jennybc, I think permission is already granted (bottom right of below screenshot).

image

And thanks for following up on this!

jennybc added some commits Dec 19, 2017

@jennybc jennybc force-pushed the PeteHaitch:master branch from 513c647 to d4711ad Dec 19, 2017

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Dec 19, 2017

@PeteHaitch Yes, indeed I can push! I don't know if I could have detected that myself, but it seemed cheeky to just start pushing into your master branch.

In any case, I think this is good to go now.

@PeteHaitch

This comment has been minimized.

Copy link
Contributor Author

PeteHaitch commented Dec 19, 2017

Excellent! Thanks for incorporating. I'll try to make cleaner patches so that future PRs aren't so much work on your end.

hook_path <- file.path(".git/hooks", hook)
write_over(base_path, hook_path, script)

This comment has been minimized.

@hadley

hadley Dec 19, 2017

Member

This made me realise usethis should have a proj_path() function for constructing paths in the current project

@hadley hadley merged commit 5480955 into r-lib:master Dec 19, 2017

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Dec 19, 2017

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.