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

Do not use pathlib for hdf5 datasets, fixes #2318 #2319

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Apr 26, 2023

fixes #2318

@maxnoe maxnoe requested review from kosack and Tobychev April 26, 2023 07:03
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this use of paths inside of h5 files tested somewhere? I took a look at io/tests/test_hdf5.py but I nothing stands out as testing the access of paths inside of a file.

@kosack
Copy link
Contributor

kosack commented May 4, 2023

Was there a reason to implement this by hand rather than just switching to using PurePosixPath as stated in the issue? That should work, right? and it is a smaller change and leverages the standard library, rather than introducing more code. With that, split_hdf5_path can be removed altogether.

Checking for a leading '/' would then be done like this:

path = PurePosixPath(path)

if path.root != '/':
    raise ValueError("HDF5 paths should be absolute (start with '/') and not relative", path)

@kosack kosack self-requested a review May 4, 2023 15:32
@maxnoe
Copy link
Member Author

maxnoe commented May 8, 2023

Was there a reason to implement this by hand rather than just switching to using PurePosixPath as stated in the issue?

I only used (and moved) the already existing functionality to fix this specific issue.

I agree that we probably should cleanup all our path handling in the hdf5 modules and PurePosixPath might be a good solution, but I'd keep that for another PR.

@maxnoe maxnoe requested a review from Tobychev May 8, 2023 08:49
@maxnoe maxnoe added this to the v0.19.1 milestone May 8, 2023
@maxnoe maxnoe merged commit 062fd96 into main May 10, 2023
@maxnoe maxnoe deleted the hdf5_path_handling branch May 10, 2023 09:13
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.

PurePath screws up HDF5 table construction on Windows systems
3 participants