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

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

Closed
boshek opened this Issue May 11, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@boshek
Contributor

boshek commented May 11, 2018

Currently use_template throws an error when one chooses not to overwrite an existing file:

usethis::use_template(template = "CONTRIBUTING.md")
Overwrite pre-existing file 'CONTRIBUTING.md'?
1: Absolutely
2: No
3: Not now
 
Selection: 3
Error: 'CONTRIBUTING.md' already exists.

Is there any appetite for a PR that would add a allow_existing argument (or whatever the name) to use_template? So that is, if a file already exists and the allow_existing argument is set to TRUE, and a user selects to not overwrite (at the prompt), use_template simply outputs a 'CONTRIBUTING.md' already exists' warning rather than an error?

The context is that we have a series of templates (CoC, README, etc) where a user, when setting up a project or package, may accept the template for some and keep existing files for others. Since use_template via write_over throws an error, this type of process where sequential asking which files need to be overwritten is currently not possible.

@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 11, 2018

Member

I agree it makes sense to be able to opt-out of overwrite and not throw an error. However, my mind is very much elsewhere right now.

Before making a PR, will you give a few bullet points re: what you plan to do so @hadley can also give a quick 👍?

The objective is to make sure the change really needs to happen in usethis vs. in your package (without forcing you to do some awkward tryCatch() thing). Let's make sure the accommodations are made in the most sensible place(s).

Member

jennybc commented May 11, 2018

I agree it makes sense to be able to opt-out of overwrite and not throw an error. However, my mind is very much elsewhere right now.

Before making a PR, will you give a few bullet points re: what you plan to do so @hadley can also give a quick 👍?

The objective is to make sure the change really needs to happen in usethis vs. in your package (without forcing you to do some awkward tryCatch() thing). Let's make sure the accommodations are made in the most sensible place(s).

@boshek

This comment has been minimized.

Show comment
Hide comment
@boshek

boshek May 11, 2018

Contributor

awkward tryCatch()

Definitely where I am at right now

Ok here is a shot at what I would try:

  • Add this into write_over here:
if (!can_overwrite(full_path)){
  ## Keep existing file AND allow_existing = FALSE (default)
  if(!allow_existing) stop(value(path), " already exists.", call. = FALSE)
  
  ## Keep existing file AND allow_existing = TRUE
  if(allow_existing) warning(value(path), " already exists.")
}
  • Then add a allow_existing argument to write_over and use_template
  • Add param documentation to allow_existing for use_template
Contributor

boshek commented May 11, 2018

awkward tryCatch()

Definitely where I am at right now

Ok here is a shot at what I would try:

  • Add this into write_over here:
if (!can_overwrite(full_path)){
  ## Keep existing file AND allow_existing = FALSE (default)
  if(!allow_existing) stop(value(path), " already exists.", call. = FALSE)
  
  ## Keep existing file AND allow_existing = TRUE
  if(allow_existing) warning(value(path), " already exists.")
}
  • Then add a allow_existing argument to write_over and use_template
  • Add param documentation to allow_existing for use_template
@jennybc

This comment has been minimized.

Show comment
Hide comment
@jennybc

jennybc May 12, 2018

Member

I don't see why this needs to stop() if interactive(), i.e. a human was presented with and made a choice. I propose you make a PR with no new argument. If !can_overwrite() and interactive() then warn or message and return FALSE from write_over().

Member

jennybc commented May 12, 2018

I don't see why this needs to stop() if interactive(), i.e. a human was presented with and made a choice. I propose you make a PR with no new argument. If !can_overwrite() and interactive() then warn or message and return FALSE from write_over().

boshek added a commit to boshek/usethis that referenced this issue May 14, 2018

@jennybc jennybc added this to the v1.4.0 milestone May 25, 2018

@jennybc jennybc changed the title from Feature request - argument to allow use_template warning on existing files to Allow user to decline to overwrite existing file, from use_template() Aug 12, 2018

@jennybc jennybc closed this in #350 Aug 13, 2018

jennybc added a commit that referenced this issue Aug 13, 2018

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

* a negative user response to overwriting a file produces a message instead of an error. Closes #348

* only modify if open = TRUE and the file is actually created

* modify test_that names to reflect warnings

* use longer form comparison

* skip R 3.1 when testing message for use_readme overwrite

* Tweak docs

Also a test to see if I can push to boshek/master

* Swap an if-else

* These functions have moved

* Use `done()` instead of `message()`

* Add expectations for file or dir in the project

* Finish adjusting to new behaviour or write_over() and use_template()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment