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 with_project() and local_project() #441

Merged
merged 11 commits into from Aug 20, 2018
Merged

Add with_project() and local_project() #441

merged 11 commits into from Aug 20, 2018

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Aug 19, 2018

Add with_project() and local_project() (and make some improvements to proj_get() / proj_set() that also make this easier.)

Remove the quiet argument from proj_get() / proj_set(). All user-facing messages go through cat_line() and as of b0df5f1, it consults the usethis.quiet option, defaulting to FALSE. Seems like progress for proj_get() / proj_set() to work the way everything else does.

I have written many versions of proj_set() at this point. How I ended up here:

  • Must support setting path = NULL. Comes up when restoring the state of "no active usethis project".
  • Must run path through proj_path_prep(), preferably just once, and prior to any user-facing messages.
  • Keep these concerns separate: NULL-ness of path, force (and quiet).

@@ -18,6 +18,7 @@ export(edit_r_environ)
export(edit_r_makevars)
export(edit_r_profile)
export(edit_rstudio_snippets)
export(local_project)
Copy link
Member Author

@jennybc jennybc Aug 20, 2018

Choose a reason for hiding this comment

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

Alternative names? local_proj() and with_proj() 🤔

Copy link
Member

@jimhester jimhester Aug 20, 2018

Choose a reason for hiding this comment

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

I think with_project() is good

proj$cur <- path
invisible(old)
}

Copy link
Member Author

@jennybc jennybc Aug 20, 2018

Choose a reason for hiding this comment

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

These are now the only 2 functions that handle proj$cur directly.

@jennybc jennybc requested review from hadley and jimhester Aug 20, 2018
jennybc added 2 commits Aug 20, 2018
`proj` is the name of the internal environment that stores active usethis project
Copy link
Member

@jimhester jimhester left a comment

Minor comments, but overall LGTM

@@ -18,6 +18,7 @@ export(edit_r_environ)
export(edit_r_makevars)
export(edit_r_profile)
export(edit_rstudio_snippets)
export(local_project)
Copy link
Member

@jimhester jimhester Aug 20, 2018

Choose a reason for hiding this comment

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

I think with_project() is good

R/proj.R Outdated
old_quiet <- options(usethis.quiet = quiet)
old_proj <- proj_set(path = path, force = force)

on.exit(proj_set(path = old_proj, force = TRUE))
Copy link
Member

@jimhester jimhester Aug 20, 2018

Choose a reason for hiding this comment

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

I maybe would just combine these together into one on.exit() call.

on.exit({
  proj_set(path = old_proj, force = TRUE)
  options(old_quiet)
})

Copy link
Member Author

@jennybc jennybc Aug 20, 2018

Choose a reason for hiding this comment

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

Yeah, that also makes the with_ and local_ code more parallel. Done.

R/proj.R Outdated
withr::defer({
proj_set(path = old_proj, force = TRUE)
options(old_quiet)
}, envir = parent.frame())
Copy link
Member

@jimhester jimhester Aug 20, 2018

Choose a reason for hiding this comment

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

Generally I think you need to expose the envir parameter for local functions, there are some cases where parent.frame() is not actually the environment you might want.

Copy link
Member Author

@jennybc jennybc Aug 20, 2018

Choose a reason for hiding this comment

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

OK I did this exactly as I see in all other withr::local_*() functions.

@jennybc jennybc merged commit c67d059 into master Aug 20, 2018
0 of 4 checks passed
@jennybc jennybc deleted the with-project branch Aug 20, 2018
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

2 participants