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

New policy for create functions #511

Merged
merged 20 commits into from Dec 4, 2018
Merged

New policy for create functions #511

merged 20 commits into from Dec 4, 2018

Conversation

hadley
Copy link
Member

@hadley hadley commented Nov 27, 2018

Create functions now preserve the active project. This shouldn't affect people using RStudio, since a new session will be started automatically, but I think this is easier to reason about

Fixes #453 — this ended up being a bigger change than I expected, but I think it's the right thing to do. I know the previous behaviour often ended up confusing me when teaching.

Create functions now preserve the active project. This shouldn't affect people using RStudio, since a new session will be started automatically, but I think this is easier to reason about

Fixes #453
@hadley hadley requested a review from jennybc Nov 27, 2018
@jennybc
Copy link
Member

@jennybc jennybc commented Dec 4, 2018

I took this out for a spin, hence the subsequent commits.

I'm not totally convinced re: not activating the new project, but I can live with it. It raises two questions for me:

  • Should we return the path to the newly created thing from create_() functions? This reveals the path after applying our path-handling policies and would make various things easier, both in terms of usage and tests.
  • If we aren't going to open the newly created thing in a new RStudio session and it's interactive, should we ask if the user would like to switch the active project and working directory? Right now people have to remember to switch both of these manually.

@hadley
Copy link
Member Author

@hadley hadley commented Dec 4, 2018

Yes, we should definitely return the path.

I'm not sure about asking. Changing the working directory just seems like such an odd thing to do. Shall we discuss on Wednesday?

@jennybc
Copy link
Member

@jennybc jennybc commented Dec 4, 2018

Maybe there should be a proj_switch(path) function or similar that activates project at path and sets working directory to that project. Right now, in a non-RStudio workflow, right after create_(), executing project_set() alone leaves things in a confusing place: old working directory but usethis is targeting the new project. This awkward situation is what we're modelling in the README of this PR.

Update: I feel even more strongly about ⬆️ now, after more use of this branch. If people have to switch to a new project manually, we need to dig a pit of success, i.e. provide a function to activate the new project and set working directory to same.

jennybc added 4 commits Dec 4, 2018
Without this change, ui_path() looks for these files in working directory (not active project) when determining if entry is a directory or not. Found this via weird trailing slash issues when workding dir != active project.
}

invisible(TRUE)
invisible(proj_get())
Copy link
Member Author

@hadley hadley Dec 4, 2018

Choose a reason for hiding this comment

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

Suggested change
invisible(proj_get())
invisible(path)

I think that's easier to reason about

Copy link
Member

@jennybc jennybc Dec 4, 2018

Choose a reason for hiding this comment

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

If we do this, we'll also need to re-process with proj_path_prep(), i.e. return invisible(proj_path_prep(path)). The reason for using what proj_get() returns is that it guarantees consistent path handling (vis-a-vis realization, expansion, normalization, etc.). This, in turn, is necessary for other path comparison things to work, e.g. try to prevent more headaches like #479.

@jennybc jennybc self-assigned this Dec 4, 2018
@jennybc
Copy link
Member

@jennybc jennybc commented Dec 4, 2018

We could also allow open = TRUE to mean "switch to new project", where that means launch in new RStudio session, if in RStudio, or change active project & working directory in current session.

But then we've almost gone around in a giant circle and come back to where we started 😕

@hadley hadley merged commit 797061e into master Dec 4, 2018
1 of 4 checks passed
@hadley hadley deleted the create-active-project branch Dec 4, 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