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

LocalStore.list_prefix adds directory prefix to all paths #2190

Closed
dcherian opened this issue Sep 16, 2024 · 4 comments
Closed

LocalStore.list_prefix adds directory prefix to all paths #2190

dcherian opened this issue Sep 16, 2024 · 4 comments
Labels
bug Potential issues with the zarr-python library
Milestone

Comments

@dcherian
Copy link
Contributor

Zarr version

v3

Numcodecs version

?

Python Version

?

Operating System

?

Installation

?

Description

list_prefix("") for a LocalStore seems inconsistent with the other stores. Discovered in #2189

'/private/var/folders/sc/hsdthq2x7mnbfvwnk7rp4xmr0000gn/T/pytest-of-deepak/pytest-854/test_zarr_hierarchy_local_0/zarr.json'

Also:

def test_list_prefix(self, store: LocalStore) -> None:
assert True

LOL

Steps to reproduce

Additional output

No response

@dcherian dcherian added the bug Potential issues with the zarr-python library label Sep 16, 2024
@jhamman jhamman added this to the 3.0.0 milestone Sep 16, 2024
@jhamman jhamman moved this to Todo in Zarr-Python - 3.0 Sep 16, 2024
@zoj613
Copy link

zoj613 commented Sep 17, 2024

In my very inexperienced eyes, this looks like the cause:

for p in (self.root / prefix).rglob("*"):
if p.is_file():
yield str(p)
to_strip = str(self.root) + "/"
for p in (self.root / prefix).rglob("*"):
if p.is_file():
yield str(p).replace(to_strip, "")

The root directory path is not stripped away and returned as is (line 206). I am not sure why the code appears to be duplicated in that function? Maybe someone who was implementing the correct code (lines 208 - 211) forgot to delete the previous block?

@jhamman
Copy link
Member

jhamman commented Sep 17, 2024

I think @d-v-b is working on a related set of fixes here: #2064

@d-v-b
Copy link
Contributor

d-v-b commented Sep 17, 2024

yep, and I can split the fixes out of #2064 into their own PR

@d-v-b
Copy link
Contributor

d-v-b commented Sep 19, 2024

#2064 is in, so I'm curious to see if the problem you observed is still around.

def test_list_prefix(self, store: LocalStore) -> None:
assert True

LOL indeed, this bit me when working on the PR because I had no idea why my changes to the localstore implementation were not making tests fail :)

@github-project-automation github-project-automation bot moved this from Todo to Done in Zarr-Python - 3.0 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Potential issues with the zarr-python library
Projects
Status: Done
Development

No branches or pull requests

4 participants