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

Change proj_activate so it accepts relative paths #954

Closed
lettucehead opened this issue Dec 7, 2019 · 2 comments
Closed

Change proj_activate so it accepts relative paths #954

lettucehead opened this issue Dec 7, 2019 · 2 comments
Labels
bug paths 🚶‍♂️ wip

Comments

@lettucehead
Copy link

@lettucehead lettucehead commented Dec 7, 2019

Hello,

Instead of going through the process of creating a reproducible example, can you just look at this diff?

diff --git a/R/proj.R b/R/proj.R
index 5ac1e5e..974b301 100644
--- a/R/proj.R
+++ b/R/proj.R
@@ -274,11 +274,11 @@ proj_activate <- function(path) {
     rstudioapi::openProject(path, newSession = TRUE)
     invisible(FALSE)
   } else {
+    proj_set(path)
     if (user_path_prep(getwd()) != path) {
       ui_done("Changing working directory to {ui_path(path, base = NA)}")
       setwd(path)
     }
-    proj_set(path)
     invisible(TRUE)
   }
 }
diff --git a/tests/testthat/test-proj.R b/tests/testthat/test-proj.R
index 5294411..22bfe4b 100644
--- a/tests/testthat/test-proj.R
+++ b/tests/testthat/test-proj.R
@@ -1,5 +1,19 @@
 context("projects")
 
+test_that("proj_activate() on relative dir", {
+  tmpdir <- file_temp()
+  tmpleaf <- basename(tmpdir)
+  tmpparent <- dirname(tmpdir)
+  on.exit(dir_delete(tmpdir))
+  dir_create(tmpdir)
+  setwd(tmpparent)
+  proj_activate(tmpleaf) # This is the important part.
+  expect_equal(
+    getwd(),
+    tmpdir
+  )
+})
+
 test_that("proj_set() errors on non-existent path", {
   expect_usethis_error(
     proj_set("abcedefgihklmnopqrstuv"),

Unfortunately the tests cannot be run for some reason, so if there are any regressions that is possibly going to need someone to look at the tests themselves.

Here is the output of the tests:

        > devtools::test()
        Loading usethis

        Attaching package: ‘testthat’

        The following object is masked from ‘package:devtools’:

                        test_file
        ✔ Setting active project to '/usr/home/r/usethis'
        Error in importIntoEnv(pkgenv, exports, nsenv, exports) :
                cannot change value of locked binding for 'proj_get'
        Calls: test ... load_all -> <Anonymous> -> export_ns -> importIntoEnv
        > traceback()
        5: importIntoEnv(pkgenv, exports, nsenv, exports)
        4: export_ns(package)
        3: pkgload::load_all(path = path, reset = reset, recompile = recompile,
                                 export_all = export_all, helpers = helpers, quiet = quiet,
                                 ...)
        2: load_all(pkg$path, quiet = TRUE)
        1: test()

Code created by my friend _@thomaslevine.com He prefers to not use proprietary software so I am giving him a hand.

If you seriously need a reprex I am sure that can be produced, but obviously it's merely a formality at this point since we already have a diff.

@malcolmbarrett
Copy link
Collaborator

@malcolmbarrett malcolmbarrett commented Mar 3, 2020

I spoke with Thomas about this issue last night, and I have a reprex that should work even when RStudio is running. (Respectfully, this is one of the reasons why reprexes are important. It took me a while to understand under what circumstances there is an issue and what the issue actually was).

Fundamentally, the issue is that proj_set() uses the working directory to set the project directory, but proj_activate() changes the working directory to path when RStudio is not detected.

Moving proj_set(), as Thomas did, does work, but there is a second, minor issue. proj_activate() checks path against the working directory, but if path is relative, comparing it to the absolute working directory will seem different even if it's not. That will then needlessly reset the working directory (

if (user_path_prep(getwd()) != path) {
)

I also think set working directory -> set project also makes more sense, in terms of execution order. My suggestion is that path get processed with proj_path_prep() instead of this line

path <- user_path_prep(path)

That will produce an absolute path and avoid the issue entirely.

One more thing: the test in this patch wouldn't work because 1) it doesn't create a project, just an empty directory and 2) it doesn't mock-disable RStudio, so it can't meaningfully run in tests run in the RStudio IDE.

Reprex:

# create a temporary directory 
parent_dir <- tempfile()
dir.create(parent_dir)
old <- setwd(parent_dir)

# create a new project
dir.create("child_dir")
file.create("child_dir/.here")
#> [1] TRUE

library(usethis)

# relative directory fails
testthat::with_mock("rstudioapi::isAvailable" = function(x) FALSE, proj_activate("child_dir"))
#> ✔ Changing working directory to 'child_dir/'
#> Error: Directory '/private/var/folders/03/9x7925g54mncswxx06wpkxl00000gn/T/RtmpaCmKIH/file396e542cd201/child_dir/child_dir' does not exist.

# reset directory and project after failed attempt
setwd(parent_dir)
proj_set(NULL)
#> ✔ Setting active project to '<no active project>'

# absolute directory succeeds
testthat::with_mock("rstudioapi::isAvailable" = function(x) FALSE, proj_activate(normalizePath("child_dir")))
#> ✔ Changing working directory to '/private/var/folders/03/9x7925g54mncswxx06wpkxl00000gn/T/RtmpaCmKIH/file396e542cd201/child_dir/'
#> ✔ Setting active project to '/private/var/folders/03/9x7925g54mncswxx06wpkxl00000gn/T/RtmpaCmKIH/file396e542cd201/child_dir'

setwd(old)
unlink(parent_dir, recursive = TRUE)

Created on 2020-03-03 by the reprex package (v0.3.0)

@lettucehead
Copy link
Author

@lettucehead lettucehead commented Mar 3, 2020

@hadley hadley added bug paths 🚶‍♂️ wip labels Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug paths 🚶‍♂️ wip
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants