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

path names on windows #100

Open
k-dominik opened this issue Apr 4, 2019 · 7 comments
Open

path names on windows #100

k-dominik opened this issue Apr 4, 2019 · 7 comments

Comments

@k-dominik
Copy link
Contributor

I'm just having a bit of fun with z5py on windows. I have noticed that you use os throughout to handle paths, which is of course nice. But this is also not compatible with h5py, where on windows, internal paths would be returned with forward slashes. (setting aside for now, that z5py also prepends the full path, as opposed to h5py where those internal paths are relative to the root.) AFAIK windows works well with with paths containing forward slashes (it even works in good old cmd).

In order to deal with both libraries more transparently, it would be great to change to something that yields forward slashes on windows. One way would be using pathlib (python stdlib).

    # in your Group class' __init__:
    self.path = pathlib.Path(path)
    
    def __iter__(self) -> str:
        for name in self.path.iterdir():
            if name.is_dir():
                # or maybe even return pathlib.Path so consumer can decide what to do
                yield name.as_posix()

I can open a PR if you think this is a valid approach.

@constantinpape
Copy link
Owner

Your suggested solution looks good. Feel free to start PR.

z5py also prepends the full path, as opposed to h5py where those internal paths are relative to the root

We should probably fix that as well. However, I am not sure what's the best way to do this, because changing this might clash with some internal functions that expect to get the full path.

@clbarnes
Copy link
Contributor

clbarnes commented Apr 4, 2019

I am all in favour of pathlib generally but z5 currently supports py2, which does not have pathlib. There is the pathlib2 backport but there are a number of cases where you can't use pathlib and pathlib2 objects interchangeably. It's also worth noting that you can't use string and Path objects interchangeably for many standard python functions until 3.6.

I am also all in favour of dropping py2 support entirely but that's just me...

In general it would be nice for z5 objects to maintain separate attributes for the file system path to the root, and the internal path. They easily can be slapped together by methods which need them: the internal path could be presented by as_posix() when it's needed on its own.

One possible sticking point here is that I think the N5 spec allows N5 "Files" to be inside other N5 "Files", so what is the internal and what is the external path is not defined by anything except the caller. Shouldn't break anything, just worth being aware of.

@constantinpape
Copy link
Owner

I am also all in favour of dropping py2 support entirely but that's just me...

Yes, I think we will drop py2 support soon, so that should not stop us from going forward with pathlib.

In general it would be nice for z5 objects to maintain separate attributes for the file system path to the root, and the internal path. They easily can be slapped together by methods which need them: the internal path could be presented by as_posix() when it's needed on its own.

Yes, I second that.

One possible sticking point here is that I think the N5 spec allows N5 "Files" to be inside other N5 "Files", so what is the internal and what is the external path is not defined by anything except the caller. Shouldn't break anything, just worth being aware of.

I think this is fine. The root path is what was passed to z5py.File.

@k-dominik
Copy link
Contributor Author

there is pathlib2 (available via conda forge) which is a backport of pathlib to python2

@constantinpape
Copy link
Owner

citing @clbarnes

There is the pathlib2 backport but there are a number of cases where you can't use pathlib and pathlib2 objects interchangeably.

@k-dominik
Copy link
Contributor Author

Sorry, shouldn't comment from my phone :/

@constantinpape
Copy link
Owner

Sorry, shouldn't comment from my phone :/

No worries.

Also, like I said py2 compatibility should not be a big concern, we will abandon it soon anyways.
(I will at least make one more release before that though).

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

No branches or pull requests

3 participants