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

Have usethis::proj_path_prep() use fs::path_abs(fs::path_expand()) instead of fs::path_real() #479

Closed
jimhester opened this issue Oct 12, 2018 · 3 comments · Fixed by #485
Labels
bug an unexpected problem or unintended behavior paths 🚶‍♂️ wip work in progress

Comments

@jimhester
Copy link
Member

jimhester commented Oct 12, 2018

usethis currently uses fs::path_real() to determine the project path, however this poses problems when the path is a mapped network drive on Windows, because the root no longer can be calculated relative to the input path.

I map my R projects folder to my windows VM, so I run into this situation often.

I propose to instead use fs::path_abs(fs::path_expand()).

usethis::proj_set("Z:/projects/remotes")
#>  Setting active project to '//vmware-host/Shared Folders/projects/remotes'
usethis::use_test()
#> Error: Open file must be in the 'R/' directory of the active package.
#> * Actual path: 'Z:/projects/remotes/R/submodule.R'
fs::path_real("Z:/projects/remotes/R/submodule.R")
#> //vmware-host/Shared Folders/projects/remotes/R/submodule.R
fs::path_abs(fs::path_expand("Z:/projects/R/submodule.R"))
#> Z:/projects/R/submodule.R

fs::path_real("~/Links")
#> C:/Users/IEUser/Links
fs::path_abs(fs::path_expand("~/Links"))
#> C:/Users/IEUser/Links

Created on 2018-10-12 by the reprex package (v0.2.0).

@jennybc
Copy link
Member

jennybc commented Oct 12, 2018

When I first did this, I did that survey of the fs path expanding/normalizing/realizing functions and how they overlap with each other (or do not).

I concluded that fs::path_real() was the best plan, but my confidence was far from 100%. I will look back at the commits to see if I recorded anything interesting.

Barring some discovery that we must discuss ... sure, I'm happy to make this switch.

@jennybc
Copy link
Member

jennybc commented Oct 12, 2018

Seems to be the most important commit/comment: 29397a6#r195969953

I see no specific reason to prefer fs::path_real() (current) over fs::path_abs(fs::path_expand()) (your proposal).

@jennybc
Copy link
Member

jennybc commented Oct 29, 2018

See #485 why this isn't quite simple. I am caught between path handling in rprojrooot (definitely uses normalizePath()) and rstudioapi (not sure what I can count on).

What if we leave proj_path_prep() as is and use it instead of path_expand_r() here?

usethis/R/r.R

Line 42 in 44dd72c

active_file <- path_expand_r(rstudioapi::getSourceEditorContext()$path)

I do not have a Windows VM at the moment to test with.

@hadley hadley added bug an unexpected problem or unintended behavior paths 🚶‍♂️ wip work in progress labels Nov 24, 2018
@jennybc jennybc modified the milestones: v1.5.0, Backlog Mar 28, 2019
@jennybc jennybc removed this from the Backlog milestone Apr 10, 2019
jennybc added a commit that referenced this issue May 3, 2019
* Realize proj path --> expand and absolutize proj path

As requested by @jimhester in #479

* Use our own prep policy

* Revert "Realize proj path --> expand and absolutize proj path"

This reverts commit bfb60d7.

* Treat paths returned by rstudioapi same as those from user
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior paths 🚶‍♂️ wip work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants