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

`use_template()` no longer errors when a user chooses not to overwrite an existing file and instead produces a message #350

Merged
merged 12 commits into from Aug 13, 2018

Conversation

Projects
None yet
3 participants
@boshek
Contributor

boshek commented May 14, 2018

Closes #348 Allow user to decline to overwrite existing file, from use_template()

  • Modified write_over so that it produces a message rather than an error when can_overwrite() is FALSE
  • Revised relevant tests to test for a message rather than an error
  • Added a single entry in the NEWS file outlining this small change (possibly not big enough to merit a NEWS item?)

boshek added some commits May 14, 2018

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek May 14, 2018

Contributor

R CMD check passing locally.

Appveyor error:

Error: package or namespace load failed for 'usethis':
.onLoad failed in loadNamespace() for 'usethis', details:
call: NULL
error: package 'backports' was installed by an R version with different internals; it needs to be reinstalled for use with this R version
Error: loading failed
Execution halted

  • possibly an old version of backports that hasn't updated yet?

travis error:
For some reason this is behaving correctly as the test is on R 3.1 that is failing:

# git2r::git2r::discover_repository() not working on R 3.1 (Travis)
skip_if(getRversion() < 3.2)

Possibly clearing caches on both would help?

Contributor

boshek commented May 14, 2018

R CMD check passing locally.

Appveyor error:

Error: package or namespace load failed for 'usethis':
.onLoad failed in loadNamespace() for 'usethis', details:
call: NULL
error: package 'backports' was installed by an R version with different internals; it needs to be reinstalled for use with this R version
Error: loading failed
Execution halted

  • possibly an old version of backports that hasn't updated yet?

travis error:
For some reason this is behaving correctly as the test is on R 3.1 that is failing:

# git2r::git2r::discover_repository() not working on R 3.1 (Travis)
skip_if(getRversion() < 3.2)

Possibly clearing caches on both would help?

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 14, 2018

Member

It looks like I'll be doing a small release of usethis soon (e.g. within the month), prompted by the backports update, so I will sort out these things once I dig in.

Member

jennybc commented May 14, 2018

It looks like I'll be doing a small release of usethis soon (e.g. within the month), prompted by the backports update, so I will sort out these things once I dig in.

@jennybc

Just one small change. Otherwise, looks good.

Next time you probably be better off to make the PR from a non-master branch in your fork.

It will be interesting to see if the CI situation has improved, when you make a new commit. Various things that have nothing to do with your PR have (hopefully) been sorted out. I pass tests fine locally with this.

Show outdated Hide outdated R/helpers.R
@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 25, 2018

Member

Upon further thought, the failure on Travis re: use_readme_rmd() is real and needs to be dealt with.

Previously, we would have stopped inside use_template() if we can't overwrite an existing README.Rmd. But now we just message and go on to execute the next block, which calls uses_git() to decide whether to add a hook. And the failure on R 3.1 is alerting to this new behaviour.

In general, you need to do a survey of all existing uses of use_template() and see if subsequent code assumes that a new template file was created. How to deal with this? It depends on how many functions are affected. Can you characterize the scope of this problem?

Member

jennybc commented May 25, 2018

Upon further thought, the failure on Travis re: use_readme_rmd() is real and needs to be dealt with.

Previously, we would have stopped inside use_template() if we can't overwrite an existing README.Rmd. But now we just message and go on to execute the next block, which calls uses_git() to decide whether to add a hook. And the failure on R 3.1 is alerting to this new behaviour.

In general, you need to do a survey of all existing uses of use_template() and see if subsequent code assumes that a new template file was created. How to deal with this? It depends on how many functions are affected. Can you characterize the scope of this problem?

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek May 28, 2018

Contributor

Next time you probably be better off to make the PR from a non-master branch in your fork

Thanks for the tip.

Happy to characterize the scope of this. Hope to have some time near the end of this week to spend on this. Will that timing work?

Contributor

boshek commented May 28, 2018

Next time you probably be better off to make the PR from a non-master branch in your fork

Thanks for the tip.

Happy to characterize the scope of this. Hope to have some time near the end of this week to spend on this. Will that timing work?

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 28, 2018

Member

Happy to characterize the scope of this. Hope to have some time near the end of this week to spend on this. Will that timing work?

No worries, I might get to it sooner or not. But we have to sort it out before we can merge this. As you might see, I now think it's connected to another issue #178.

Member

jennybc commented May 28, 2018

Happy to characterize the scope of this. Hope to have some time near the end of this week to spend on this. Will that timing work?

No worries, I might get to it sooner or not. But we have to sort it out before we can merge this. As you might see, I now think it's connected to another issue #178.

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek May 28, 2018

Contributor

Here is a bare bones visualization that I'll leave here as a reference for myself (or whomever ultimately tackles this):

image

Contributor

boshek commented May 28, 2018

Here is a bare bones visualization that I'll leave here as a reference for myself (or whomever ultimately tackles this):

image

@ateucher

This comment has been minimized.

Show comment
Hide comment
@ateucher

ateucher May 30, 2018

I hope you don't mind me piping in here... With this PR, use_template() (via write_over()) returns FALSE with a message now instead of failing if can_overwrite is FALSE … but that behaviour existed already when the old file and the new file were identical. So is this just exposing something that was previously untested?

So the question is (specifically for use_readme_rmd()) would you want the hook to be added if the README.Rmd already existed and wasn't overwritten? The existing behaviour would add the hook if the file existed already but was identical to the template. Is the new case really any different?

My apologies if I'm off base or missed something obvious!

ateucher commented May 30, 2018

I hope you don't mind me piping in here... With this PR, use_template() (via write_over()) returns FALSE with a message now instead of failing if can_overwrite is FALSE … but that behaviour existed already when the old file and the new file were identical. So is this just exposing something that was previously untested?

So the question is (specifically for use_readme_rmd()) would you want the hook to be added if the README.Rmd already existed and wasn't overwritten? The existing behaviour would add the hook if the file existed already but was identical to the template. Is the new case really any different?

My apologies if I'm off base or missed something obvious!

@ateucher

This comment has been minimized.

Show comment
Hide comment
@ateucher

ateucher May 30, 2018

This is some heavy yak-shaving, and possibly not very helpful, but here are the functions that call use_template() which have meaningful code that comes after the call to use_template().

Most of them are just todo()/code_block() messages, except for:

  • use_travis() and use_appveyor() call their respective *_activate() and use_*_badge() functions.
  • use_readme_rmd() we know about
  • use_rstudio() calls use_gitignore(), and use_build_ignore() if it's a package.
  • use_tidy_ci() and use_rmarkdown_template() call use_template() multiple times.
library(tidyverse)
library(igraph, quietly = TRUE, warn.conflicts = FALSE)
library(usethis)
# devtools::install_github("ropenscilabs/revtools")

find_code_after_use_template <- function(x) {
  x <- getFromNamespace(x, "usethis")
  fn_body <- body(x)
# find where use_template is called
  ut_block_no <- grep("use_template", fn_body)
  if (length(ut_block_no) > 1) {
    return("More than one call to use_template. Check this one out more carefully")
  }
# if call to use_template is the last line, or the line after the call is a return-type statement
  if (ut_block_no == length(fn_body) || grepl("^return|^invisible\\(", fn_body[ut_block_no + 1])) {
    return(character(0))
  }
# return the rest of the function body
  as.character(fn_body[seq(ut_block_no + 1, length(fn_body))])
}

# Get character vector of exported function names that call use_template
funs_that_call_use_template <- revtools:::create_package_igraph("~/dev/packages/usethis/") %>% 
  as_long_data_frame() %>% 
  filter(to_name == "use_template") %>% 
  pull(from_name)

map(funs_that_call_use_template, find_code_after_use_template) %>% 
  set_names(funs_that_call_use_template) %>% 
  Filter(length, .)
#> $use_travis
#> [1] "travis_activate(browse)" "use_travis_badge()"     
#> [3] "invisible(TRUE)"        
#> 
#> $use_appveyor
#> [1] "appveyor_activate(browse)" "use_appveyor_badge()"     
#> [3] "invisible(TRUE)"          
#> 
#> $use_code_of_conduct
#> [1] "todo(\"Don't forget to describe the code of conduct in your README.md:\")"                                                                                                                 
#> [2] "code_block(\"Please note that this project is released with a [Contributor Code of Conduct](CODE_OF_CONDUCT.md).\", \"By participating in this project you agree to abide by its terms.\")"
#> 
#> $use_pipe
#> [1] "todo(\"Run \", code(\"document()\"))"
#> 
#> $use_readme_rmd
#> [1] "if (uses_git()) {\n    use_git_hook(\"pre-commit\", render_template(\"readme-rmd-pre-commit.sh\"))\n}"
#> [2] "invisible(TRUE)"                                                                                      
#> 
#> $use_revdep
#> [1] "todo(\"Run checks with \", code(\"revdepcheck::revdep_check(num_workers = 4)\"))"
#> 
#> $use_rmarkdown_template
#> [1] "More than one call to use_template. Check this one out more carefully"
#> 
#> $use_rstudio
#> [1] "use_git_ignore(\".Rproj.user\")"                                                                     
#> [2] "if (is_package()) {\n    use_build_ignore(c(paste0(project_name(), \".Rproj\"), \".Rproj.user\"))\n}"
#> [3] "invisible(TRUE)"                                                                                     
#> 
#> $use_tidy_ci
#> [1] "More than one call to use_template. Check this one out more carefully"
#> 
#> $use_tidy_eval
#> [1] "todo(\"Run document()\")"
#> 
#> $use_tidy_coc
#> [1] "todo(\"Don't forget to describe the code of conduct in your README.md:\")"                                                                                                                         
#> [2] "code_block(\"Please note that this project is released with a [Contributor Code of Conduct](.github/CODE_OF_CONDUCT.md).\", \"By participating in this project you agree to abide by its terms.\")"

Created on 2018-05-30 by the reprex package (v0.2.0).

ateucher commented May 30, 2018

This is some heavy yak-shaving, and possibly not very helpful, but here are the functions that call use_template() which have meaningful code that comes after the call to use_template().

Most of them are just todo()/code_block() messages, except for:

  • use_travis() and use_appveyor() call their respective *_activate() and use_*_badge() functions.
  • use_readme_rmd() we know about
  • use_rstudio() calls use_gitignore(), and use_build_ignore() if it's a package.
  • use_tidy_ci() and use_rmarkdown_template() call use_template() multiple times.
library(tidyverse)
library(igraph, quietly = TRUE, warn.conflicts = FALSE)
library(usethis)
# devtools::install_github("ropenscilabs/revtools")

find_code_after_use_template <- function(x) {
  x <- getFromNamespace(x, "usethis")
  fn_body <- body(x)
# find where use_template is called
  ut_block_no <- grep("use_template", fn_body)
  if (length(ut_block_no) > 1) {
    return("More than one call to use_template. Check this one out more carefully")
  }
# if call to use_template is the last line, or the line after the call is a return-type statement
  if (ut_block_no == length(fn_body) || grepl("^return|^invisible\\(", fn_body[ut_block_no + 1])) {
    return(character(0))
  }
# return the rest of the function body
  as.character(fn_body[seq(ut_block_no + 1, length(fn_body))])
}

# Get character vector of exported function names that call use_template
funs_that_call_use_template <- revtools:::create_package_igraph("~/dev/packages/usethis/") %>% 
  as_long_data_frame() %>% 
  filter(to_name == "use_template") %>% 
  pull(from_name)

map(funs_that_call_use_template, find_code_after_use_template) %>% 
  set_names(funs_that_call_use_template) %>% 
  Filter(length, .)
#> $use_travis
#> [1] "travis_activate(browse)" "use_travis_badge()"     
#> [3] "invisible(TRUE)"        
#> 
#> $use_appveyor
#> [1] "appveyor_activate(browse)" "use_appveyor_badge()"     
#> [3] "invisible(TRUE)"          
#> 
#> $use_code_of_conduct
#> [1] "todo(\"Don't forget to describe the code of conduct in your README.md:\")"                                                                                                                 
#> [2] "code_block(\"Please note that this project is released with a [Contributor Code of Conduct](CODE_OF_CONDUCT.md).\", \"By participating in this project you agree to abide by its terms.\")"
#> 
#> $use_pipe
#> [1] "todo(\"Run \", code(\"document()\"))"
#> 
#> $use_readme_rmd
#> [1] "if (uses_git()) {\n    use_git_hook(\"pre-commit\", render_template(\"readme-rmd-pre-commit.sh\"))\n}"
#> [2] "invisible(TRUE)"                                                                                      
#> 
#> $use_revdep
#> [1] "todo(\"Run checks with \", code(\"revdepcheck::revdep_check(num_workers = 4)\"))"
#> 
#> $use_rmarkdown_template
#> [1] "More than one call to use_template. Check this one out more carefully"
#> 
#> $use_rstudio
#> [1] "use_git_ignore(\".Rproj.user\")"                                                                     
#> [2] "if (is_package()) {\n    use_build_ignore(c(paste0(project_name(), \".Rproj\"), \".Rproj.user\"))\n}"
#> [3] "invisible(TRUE)"                                                                                     
#> 
#> $use_tidy_ci
#> [1] "More than one call to use_template. Check this one out more carefully"
#> 
#> $use_tidy_eval
#> [1] "todo(\"Run document()\")"
#> 
#> $use_tidy_coc
#> [1] "todo(\"Don't forget to describe the code of conduct in your README.md:\")"                                                                                                                         
#> [2] "code_block(\"Please note that this project is released with a [Contributor Code of Conduct](.github/CODE_OF_CONDUCT.md).\", \"By participating in this project you agree to abide by its terms.\")"

Created on 2018-05-30 by the reprex package (v0.2.0).

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek Jun 1, 2018

Contributor

Using @ateucher's handy scoping:

use_travis and use_appveyor

A user with an existing *.yml file here can safely choose not to overwrite because neither the active and badge functions depend on use_template failing when an overwrite does not occur

use_readme_rmd

I think the question here identified by @ateucher is:

would you want the hook to be added if the README.Rmd already existed and wasn't overwritten
I think you would right?

I have skipped the readme_rmd test if R is less than 3.1 because it was trying to use git2r::::discover_repository() in the test now which fails on travis. Hopefully this fixes the build fail on 3.1.

use_rstudio

The main impact I see here is that calling create_project() or create_package() from a directory that has an existing .Rproj file will now ask you if you want to overwrite it.

This seems a little redundant so one could only call use_template() if an .Rproj is absent. Would this be desirable?

use_rmarkdown_template & use_tidy_ci.

This functions without any adverse downstream effects and actually provides greater control if one wants to replace the .yaml but not the rmarkdown template (or vice versa).

Contributor

boshek commented Jun 1, 2018

Using @ateucher's handy scoping:

use_travis and use_appveyor

A user with an existing *.yml file here can safely choose not to overwrite because neither the active and badge functions depend on use_template failing when an overwrite does not occur

use_readme_rmd

I think the question here identified by @ateucher is:

would you want the hook to be added if the README.Rmd already existed and wasn't overwritten
I think you would right?

I have skipped the readme_rmd test if R is less than 3.1 because it was trying to use git2r::::discover_repository() in the test now which fails on travis. Hopefully this fixes the build fail on 3.1.

use_rstudio

The main impact I see here is that calling create_project() or create_package() from a directory that has an existing .Rproj file will now ask you if you want to overwrite it.

This seems a little redundant so one could only call use_template() if an .Rproj is absent. Would this be desirable?

use_rmarkdown_template & use_tidy_ci.

This functions without any adverse downstream effects and actually provides greater control if one wants to replace the .yaml but not the rmarkdown template (or vice versa).

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek Jun 29, 2018

Contributor

@jennybc Curious is it generally considered my responsibility to fix the merge conflicts that have arisen since the PR was originally submitted or is that usually left to the maintainer (you in this case)?

Contributor

boshek commented Jun 29, 2018

@jennybc Curious is it generally considered my responsibility to fix the merge conflicts that have arisen since the PR was originally submitted or is that usually left to the maintainer (you in this case)?

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Jun 29, 2018

Member

You can leave it with me @boshek.

Member

jennybc commented Jun 29, 2018

You can leave it with me @boshek.

Tweak docs
Also a test to see if I can push to boshek/master
@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Aug 9, 2018

Member

@boshek I'm working on this now and will try to finish things up in your PR. This means I'm going to be pushing to the branch you made it from (and have already done so). In future, you probably want to work in a non-master branch for PRs, but we'll just carry on with this.

Are OK to just let me take over master branch of your fork for a little while?

Member

jennybc commented Aug 9, 2018

@boshek I'm working on this now and will try to finish things up in your PR. This means I'm going to be pushing to the branch you made it from (and have already done so). In future, you probably want to work in a non-master branch for PRs, but we'll just carry on with this.

Are OK to just let me take over master branch of your fork for a little while?

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek Aug 9, 2018

Contributor

@jennybc Fine by me.

  • I could, alternatively, close everything down and re-submit a clean PR if that were the path of least resistance at this point.
  • Happy to further contribute as well if any tasks can be farmed out
Contributor

boshek commented Aug 9, 2018

@jennybc Fine by me.

  • I could, alternatively, close everything down and re-submit a clean PR if that were the path of least resistance at this point.
  • Happy to further contribute as well if any tasks can be farmed out

jennybc added some commits Aug 9, 2018

Merge branch 'master' into boshek-master
# Conflicts:
#	NEWS.md
#	R/helpers.R
#	R/write.R
#	tests/testthat/test-use-readme.R
#	tests/testthat/test-use-template.R
@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Aug 12, 2018

Member

@boshek I think this is done. I'm just starting a vacation would like to wrap up this release before I do a complete mental check out.

Will you sanity check this again, both by taking a look at the code I pushed and by confirming it still behaves the way you want? I'll be working through my other pre-release checklist items in parallel.

Thanks!

Member

jennybc commented Aug 12, 2018

@boshek I think this is done. I'm just starting a vacation would like to wrap up this release before I do a complete mental check out.

Will you sanity check this again, both by taking a look at the code I pushed and by confirming it still behaves the way you want? I'll be working through my other pre-release checklist items in parallel.

Thanks!

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Aug 12, 2018

Member

@ateucher Review by you, instead or in addition, is also welcome!

Member

jennybc commented Aug 12, 2018

@ateucher Review by you, instead or in addition, is also welcome!

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek Aug 13, 2018

Contributor

@jennybc Looks great to me on both counts. Nothing odourous as far as I can see and the behaviour is exactly what I was after.

Note: It isn't totally clear to me how this, this and this commit aren't in the boshek-master branch but are still part of the PR here. I assume this reflect my lack of understanding but am flagging this just in case.

Contributor

boshek commented Aug 13, 2018

@jennybc Looks great to me on both counts. Nothing odourous as far as I can see and the behaviour is exactly what I was after.

Note: It isn't totally clear to me how this, this and this commit aren't in the boshek-master branch but are still part of the PR here. I assume this reflect my lack of understanding but am flagging this just in case.

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc Aug 13, 2018

Member

Thanks for taking a look!

Note: It isn't totally clear to me how this, this and this commit aren't in the boshek-master branch but are still part of the PR here. I assume this reflect my lack of understanding but am flagging this just in case.

That's a branch in this repo that I created by mistake, in some sense. I will delete it. The branch that matters for the PR is the master branch in your fork, which does have these commits.

Member

jennybc commented Aug 13, 2018

Thanks for taking a look!

Note: It isn't totally clear to me how this, this and this commit aren't in the boshek-master branch but are still part of the PR here. I assume this reflect my lack of understanding but am flagging this just in case.

That's a branch in this repo that I created by mistake, in some sense. I will delete it. The branch that matters for the PR is the master branch in your fork, which does have these commits.

@jennybc jennybc merged commit 3046fde into r-lib:master Aug 13, 2018

3 of 4 checks passed

codecov/patch 55.55% of diff hit (target 60.77%)
Details
codecov/project 60.75% (-0.03%) compared to a625cce
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment