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

Add use_blank_slate() #139

Merged
merged 24 commits into from
Dec 22, 2017
Merged

Add use_blank_slate() #139

merged 24 commits into from
Dec 22, 2017

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Dec 20, 2017

Closes #133.

Implements use_freshstarts() use_blank_slate(), to suppress save/reload of user's workspace upon R restarts.

Handles scope = "project" but not scope = "user". Reasons given in #133 (comment).

I needed to use work done in PRs #145 (Make scoped_temporary_package() easier to work with) and #146 (Less promiscuous Rbuildignoring) so those need approval first.

@jennybc jennybc requested a review from hadley December 20, 2017 06:52
R/rstudio.R Outdated
scope <- match.arg(scope)

if (scope == "user") {
message(
Copy link
Member Author

@jennybc jennybc Dec 20, 2017

Choose a reason for hiding this comment

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

I suspect this appalls you. Happy to implement in a more usethis-style.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, just use todo() instead of message().

Copy link
Member Author

Choose a reason for hiding this comment

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

done 0dbc25a

R/rstudio.R Outdated
rproj_path(),
list(RestoreWorkspace = "No", SaveWorkspace = "No")
)
write_utf8(file.path(proj_get(), rproj_path()), rproj_options)
Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer write_over() but I don't want to ask user's permission.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine

R/rstudio.R Outdated
write_utf8(file.path(proj_get(), rproj_path()), rproj_options)
restart_rstudio(
"A restart of RStudio is required to activate fresh starts"
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed restart is necessary for this to take effect.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just "Restart RStudio for a fresh state?"

R/rstudio.R Outdated
#' @inheritParams edit
#'
#' @export
use_freshstarts <- function(scope = c("user", "project")) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be use_fresh_starts()?

Copy link
Member

Choose a reason for hiding this comment

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

Or use_blank_slate()? use_tabula_rasa()?

R/rstudio.R Outdated
#' starts provide timely feedback that encourages the development of scripts
#' that are complete and self-contained.
#'
#' Only `use_freshstarts("project")` is implemented so far, since RStudio
Copy link
Member

Choose a reason for hiding this comment

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

implemented -> automated

R/rstudio.R Outdated
#'
#' Only `use_freshstarts("project")` is implemented so far, since RStudio
#' currently only supports modification of user-level or global options via the
#' user interface.
Copy link
Member

Choose a reason for hiding this comment

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

Link to blog post for more info?

R/rstudio.R Outdated
scope <- match.arg(scope)

if (scope == "user") {
message(
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, just use todo() instead of message().

R/rstudio.R Outdated
rproj_path(),
list(RestoreWorkspace = "No", SaveWorkspace = "No")
)
write_utf8(file.path(proj_get(), rproj_path()), rproj_options)
Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine

R/rstudio.R Outdated
write_utf8(file.path(proj_get(), rproj_path()), rproj_options)
restart_rstudio(
"A restart of RStudio is required to activate fresh starts"
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just "Restart RStudio for a fresh state?"

R/rstudio.R Outdated
@@ -32,3 +95,37 @@ in_rstudio <- function(base_path = proj_get()) {

normalizePath(proj) == normalizePath(base_path)
}

build_rproj <- function(file, fields) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see a bunch of unit tests for this, since it's fairly high risk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tests added. This is ready for another look.

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to use work done in PRs #145 (Make scoped_temporary_package() easier to work with) and #146 (Less promiscuous Rbuildignoring) so those need approval first.

@hadley
Copy link
Member

hadley commented Dec 20, 2017

Let's call it use_blank_slate()

* scoped-temporary-things:
  Prefer capture_output() to capture.output()
  Make scoped_temporary_package callable from the console
  Move scoped_temporary_package() to test dir
* careful-rbuildignore:
  More specific pattern re: Rbuildignoring .Rproj files
  Remove .Rbuildignore from .gitignore
  More specific Rbuildignore for usethis.Rproj
  Test we can find templates vulnerable to Rbuildignore
@jennybc jennybc changed the title Add use_freshstarts() Add use_blank_slate() Dec 22, 2017
@@ -1,4 +1,4 @@
^.*\.Rproj$
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't RStudio modify this when you re-open?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not for me. Does it for you?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I could've sworn it did in the past, but not this time!

Copy link
Member Author

Choose a reason for hiding this comment

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

I have even checked with a full "new Project (that is a package) from GitHub" workflow. And RStudio doesn't modify this in .Rbuildignore

I've come to the conclusion that the problem of the missing template was self-inflicted. Although maybe that .Rbuildignore line was originally inspired by RStudio?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed x2

expect_identical(rproj, paste0(basename(dir), ".Rproj"))
})

test_that("a non-RStudio project is recognized", {
Copy link
Member

Choose a reason for hiding this comment

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

is not recognised?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok changed

list()
)
writeLines(serialize_rproj(rproj), tmp)
expect_equivalent(
Copy link
Member

Choose a reason for hiding this comment

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

Why not expect_equal() on the lines themselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok changed

@jennybc
Copy link
Member Author

jennybc commented Dec 22, 2017

@hadley look again?

R/rstudio.R Outdated
@@ -13,12 +13,88 @@ use_rstudio <- function() {

use_git_ignore(".Rproj.user")
if (is_package()) {
use_build_ignore(c("^.*\\.Rproj$", "^\\.Rproj\\.user$"), escape = FALSE)
use_build_ignore(
c(
Copy link
Member

Choose a reason for hiding this comment

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

I think you could now make this:

use_build_ignore(c(paste0(project_name(), "\.Rproj"), ".Rproj.user"))

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, changed

[skip ci]
@jennybc jennybc merged commit 7f5dee2 into master Dec 22, 2017
@jennybc jennybc deleted the use-freshstarts branch December 22, 2017 19:15
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.

2 participants