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 first version of UNCPath #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add first version of UNCPath #12

wants to merge 2 commits into from

Conversation

jannefiluren
Copy link

Work in progress

@codecov-io
Copy link

codecov-io commented Aug 25, 2018

Codecov Report

Merging #12 into master will decrease coverage by 5.51%.
The diff coverage is 2.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   76.37%   70.86%   -5.52%     
==========================================
  Files           8        9       +1     
  Lines         419      453      +34     
==========================================
+ Hits          320      321       +1     
- Misses         99      132      +33
Impacted Files Coverage Δ
src/FilePathsBase.jl 40% <ø> (ø) ⬆️
src/uncpath.jl 2.94% <2.94%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d57a98b...520a96b. Read the comment docs.

@rofinn
Copy link
Owner

rofinn commented Sep 3, 2018

I've commented on julia-vscode/julia-vscode#490 to get a clearer understanding of the specific requirements. Having a better picture of your requirements and what level of UNC support needed should help us:

  1. Narrow down the scope of this PR
  2. Determine what we should be testing for this path type
  3. Determine where the code should eventually live

@davidanthoff
Copy link
Contributor

So I think we should not introduce a new type here, but instead just represent them as WindowsPath, which is what python does. In my mind we "just" need to add logic here to properly parse them, and then make sure all the other functions work properly with them.

@jannefiluren
Copy link
Author

jannefiluren commented Sep 6, 2018

I have no preferences how to solve this problem. What I see is that UNC paths seems to be handled poorly with Julia base too. Here is one example:

julia> cd("//hdata/fou/jmg")

julia> pwd()
"\\\\hdata\\fou\\jmg"

julia> splitdrive(pwd())    # Seems wrong to me
("\\\\hdata\\fou", "\\jmg")

Should I open an issue at Julia base for this stuff, or is it not supposed to work?

Furthermore, when writing this PR, I had this issue:

julia> UNC_PATH_START = "\\\\"
"\\\\"

julia> joinpath(UNC_PATH_START, "hdata", "fou")   # Seems correct
"\\\\hdata\\fou"

julia> joinpath(UNC_PATH_START, "hdata", "fou", "jmg")   # Not correct
"\\\\hdata\\foujmg"

The \\\\ is not a part of the path so perhaps that should not work either.

I "solved" this issue in the PR using this line of code instead:

julia> folder = UNC_PATH_START * joinpath("hdata", "fou", "jmg")
"\\\\hdata\\fou\\jmg"

julia> isdir(folder)
true

But that seems more like a hack than a good solution. How would you like me to proceed with this PR? Happy for any advice.

@jannefiluren
Copy link
Author

@rofinn and @davidanthoff:

Just some info: I just read this thread on discourse. It might be relevant for handling windows paths better. Look in particular at this pull request. Also look at my comment above.

@davidanthoff
Copy link
Contributor

I think I would just try to copy the semantics from https://github.com/python/cpython/blob/master/Lib/pathlib.py over to here...

@abx78
Copy link

abx78 commented Aug 20, 2021

Does this work? There are a few packages that cannot be used in Windows corporate environments because depending on this

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.

5 participants