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

SSHFileSystem._info should strip the protocol from the path #43

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

ryaminal
Copy link
Contributor

This is based on some pain points i was having while using GenericFileSystem.
Looks like other FS implementations also strip first. Seems like a "safe" thing to do.

pytest passes... for what it's worth.

This is based on some pain points i was having and looking at other FS impls.
@ryaminal ryaminal changed the title [WIP] SSHFileSystem._info should strip the protocol from the path SSHFileSystem._info should strip the protocol from the path Apr 17, 2024
@shcheklein
Copy link
Collaborator

@ryaminal

This is based on some pain points i was having while using GenericFileSystem.

could you please give more details?

Looks like other FS implementations also strip first.

could you please share some links? thanks!

@ryaminal
Copy link
Contributor Author

@shcheklein, here are some answers to your questions.

Pain points

The pain points with sshfs and GenericFileSystem is sort of explained here.
To summarize: because the path passed to _info contained the protocol the isdir check in GenericFileSystem would fail. Once I added the _strip_protocol from this PR that problem went away.

Examples from other FS impls that strip in _info

@shcheklein
Copy link
Collaborator

Thanks @ryaminal for the context.

@martindurant @efiop please merge it and release when you have time.

@efiop efiop merged commit 807e00b into fsspec:main Apr 22, 2024
11 checks passed
@ryaminal ryaminal deleted the patch-1 branch April 22, 2024 13:26
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.

3 participants