search

fsspec / universal_pathlib

pathlib api extended to use fsspec backends
MIT License
251 stars 44 forks source link

Consolidate UPath, test fsspec local #32

Closed brl0 closed 3 years ago

brl0 commented 3 years ago

The primary goal here is to consolidate UniversalPath and UPath.

This helps with some down stream usage.

This also adds testing for fsspec using file:// uri, which it turns out seem to be failing on Windows currently, so skipping those for now.

To my knowledge, there shouldn't be any significant functionality differences, other than no longer needing the UniversalPath class.

If this approach is generally acceptable, the examples notebook will need to be updated as well.

review-notebook-app[bot] commented 3 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

andrewfulton9 commented 3 years ago

This is great @brl0! I didn't realize you could get around circular imports like this. That's why I initially split UniversalPath from UPath. Could you run nox from the universal_pathlib directory and commit/push up the changes to get the formatting conformed to the rest of the code?

brl0 commented 3 years ago

Thanks @andrewfulton9, I appreciate the feedback! I figured that was the reason for the split, it was definitely tricky to work though the circular imports, and I was close to giving up. As usual, all credit goes to stack overflow for guiding the way. :)

I haven't updated the notebook outputs yet. I am thinking that might be better to do in another PR, in part because this one is pretty large already, and in part because I am running into a couple of issues when trying to do so. The issues I noticed were that the UPath untested default warning appears, which we may want to filter or suppress, since this notebook is the primary user documentation. Also, the readme.exists() cell is now returning False, so it looks like something somewhere may have broken, although I don't think it is in this PR, I tested on main and the issue still happens, so I will create an issue to address that.

Since this is already so large, I am holding off on any other changes, clean up, etc for another PR.

I think this PR is ready for your review. Let me know if you have any questions, concerns, etc.

andrewfulton9 commented 3 years ago

Merged! Thanks @brl0