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

Assume git users can undo #1424

Merged
merged 9 commits into from
Apr 19, 2021
Merged

Assume git users can undo #1424

merged 9 commits into from
Apr 19, 2021

Conversation

hadley
Copy link
Member

@hadley hadley commented Apr 16, 2021

Fixes #1376

This is a very simple change but has big consequences. I think it's a big UI improvement for us, and at most only a minor inconvenience for others. What do you think?

@jimhester
Copy link
Member

jimhester commented Apr 16, 2021

Should we have some way to opt-in e.g. if you wanted a safe-mode version of usethis that would prompt about overwriting, even if you are using git?

Or alternatively could we leave the defaults as-is, and just provide a way to globally opt out, e.g. an envvar or option we could put in our .Rprofile.

@jennybc
Copy link
Member

jennybc commented Apr 16, 2021

very simple change but has big consequences

No joke!

I have some concern because we project an attitude of "just get started with your first package, it's easy!" and that could easily happen around the same time as "just get started with Git and do very basic stuff" (which probably doesn't include resets).

So there could be people new to both Git and package development, who feel like we sent them down that path. And now we do this!

If we do it, we should provide a function or other support for the relevant "git undo".

@hadley
Copy link
Member Author

hadley commented Apr 16, 2021

Ok, since you both have concerns that this is too much (and I agree), I'll make this opt-in so that you also have to set some option.

@hadley
Copy link
Member Author

hadley commented Apr 16, 2021

Ok, take a look now and let me know if you have any better ideas for the option name.

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NEWS.md Outdated Show resolved Hide resolved
@hadley hadley requested a review from jennybc April 19, 2021 14:06
Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed some minor wording changes.

It feels like we should add a test. If you've moved on to something else @hadley, tell me and add that to finish this.

NEWS.md Outdated Show resolved Hide resolved
vignettes/articles/usethis-setup.Rmd Outdated Show resolved Hide resolved
Co-authored-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>
@hadley
Copy link
Member Author

hadley commented Apr 19, 2021

Sure, that'd be great, thanks!

@jennybc
Copy link
Member

jennybc commented Apr 19, 2021

Observation about this PR now that I'm writing tests:

We have been trying to keep low-level helpers like write_over() (exported) and can_overwrite()(not exported) "project-agnostic". I.e. the caller is responsible for providing the path and these functions don't know about projects.

Main principle: we don't want calls to these low-level helpers to activate or change the active project.

This PR currently violates this because it does write_over() -> can_overwrite() -> uses_git() -> proj_get(). We've run up against this before with write_utf8() and write_union(). These dilemmas are discussed in the principles document:

https://github.com/r-lib/usethis/blob/master/principles.md#helpers-and-the-active-project

I think this PR need to be adjusted similar to those other cases, such that write_over() can't trigger project activation.

What should we do if the active project uses Git, but I write_over(path, ...) a path outside the project? This is an edge case for usethis, because we're so project-focused, but still.

@hadley
Copy link
Member Author

hadley commented Apr 19, 2021

Can we just base the git check on the file path vs. the active project?

@jennybc
Copy link
Member

jennybc commented Apr 19, 2021

Can we just base the git check on the file path vs. the active project?

I will explore this, but yeah sounds like a good solution.

@jimhester
Copy link
Member

jimhester commented Apr 19, 2021

I would be tempted to remove the git check entirely if it is complicated to do correctly, e.g. if usethis.overwrite == TRUE always overwrite without asking.

The population of people who know enough about usethis to change the default but also aren't using git for their projects seems pretty small to me.

@jennybc
Copy link
Member

jennybc commented Apr 19, 2021

It's not too hard to check if path is in a Git repo, without tickling the active project.

If no one has anything else to say, I'll merge this in a bit.

@jennybc jennybc merged commit 0d181f6 into master Apr 19, 2021
@jennybc jennybc deleted the overwrite-git branch April 19, 2021 18:25
@jennybc
Copy link
Member

jennybc commented Apr 19, 2021

Merged.

I'd urge anyone who's listening to consider setting usethis.overwrite = TRUE in .Rprofile, so we accumulate some real-world usage. I have just done so myself.

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.

None yet

3 participants