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

Updating the example notebook #34

Closed
brl0 opened this issue Aug 14, 2021 · 2 comments
Closed

Updating the example notebook #34

brl0 opened this issue Aug 14, 2021 · 2 comments

Comments

@brl0
Copy link
Contributor

brl0 commented Aug 14, 2021

@andrewfulton9,

As mentioned in PR #32, the notebook output needs to be updated, but there were a couple of issues, as stated in a comment to the PR:

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.

In addition to those issues, I think it would be worth clarifying this point:

Some filesystems may require extra imports to use.

Since fsspec tries to instantiate registered implementations as needed, I think it would be more clear to state something like:

Some filesystems may require additional packages to be installed.

The import of s3fs is not necessary and may cause confusion about whether target filesystem packages need to be imported prior to usage with UPath.

One idea I had is that it might be a handy reference to import and cleanly print the known fsspec implementations at the end of the notebook.

I'd be interested in any feedback. Hopefully I'll be able to submit another PR sometime soon with some of these changes.

@brl0
Copy link
Contributor Author

brl0 commented Aug 14, 2021

I also wanted to mention it might be a good idea to include the notebook in tests.

@ap--
Copy link
Collaborator

ap-- commented May 12, 2023

This has been fixed via #96

Thank you @brl0 ❤️

@ap-- ap-- closed this as completed May 12, 2023
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

No branches or pull requests

2 participants