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

Consolidate UPath, test fsspec local #32

Merged
merged 9 commits into from
Aug 17, 2021
Merged

Conversation

brl0
Copy link
Contributor

@brl0 brl0 commented Aug 11, 2021

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
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@andrewfulton9
Copy link
Collaborator

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
Copy link
Contributor Author

brl0 commented Aug 14, 2021

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
Copy link
Collaborator

Merged! Thanks @brl0

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.

2 participants