search

laminlabs / lamindb-setup

Setup & configure LaminDB.
Apache License 2.0
4 stars 2 forks source link

🎨 Remove call to `create_path` inside `path.view_tree` #618

Closed mukund109 closed 7 months ago

mukund109 commented 8 months ago

view_tree is only being used as a method for the Path (and UPath) classes. There aren't any places where its being called with a str argument.

I've modified it to not accept str arguments anymore and removed the call to create_path.

This fixes https://github.com/laminlabs/lamindb/issues/1407

falexwolf commented 8 months ago

Thank you, Mukundh! :D

I think it's correct that create_path is removed, but I'd type this with UPath instead of with Path.

Let's see what @Koncopd says.

mukund109 commented 8 months ago

Oh I figured this method is meant to apply to pathlib.Path and its sub-classes:

https://github.com/laminlabs/lamindb-setup/blob/main/lamindb_setup/dev/upath.py#L367

falexwolf commented 8 months ago

Yeah! And also to UPath: https://github.com/laminlabs/lamindb-setup/blob/8a55c63fc42fdb631c6e2ecf6414e474865aedae/lamindb_setup/dev/upath.py#L365

Koncopd commented 7 months ago

Ye, if it is only used as a method of UPath or Path, it should be fine. @falexwolf do we use it on its own anywhere, not like ln.UPath(path_str).view_tree(), but view_tree(path_str)? I don't see it.

mukund109 commented 7 months ago

Yeah! And also to UPath:

https://github.com/laminlabs/lamindb-setup/blob/8a55c63fc42fdb631c6e2ecf6414e474865aedae/lamindb_setup/dev/upath.py#L365

Isn't UPath a subclass of Path? Making path: Path equivalent to path: Union[Path, UPath]

I can see in other parts of the code-base also that you've explicitly typed Union[Path, UPath]. Couldn't quite figure out the reason for this.

falexwolf commented 7 months ago

No, we don't! It's only used via path.view_tree(), so, we need no conversion from str and don't need to call create_path.

I think this is reminiscent of the old implementation.

Isn't UPath a subclass of Path? Making path: Path equivalent to path: Union[Path, UPath]

If this is the case, now, then it's all good! Previously, coming from CloudPath, we always proxied a cloud path via UPath and a local path via Path. But since upath 0.1 with the proper type system, I think we might not need this anymore.

Let's merge this PR as it is now!

falexwolf commented 7 months ago

Merging this, now! In case it breaks tests, I'll fix it.

falexwolf commented 7 months ago

Thanks a ton for this, Mukundh!