Skip to content

Conversation

@eguir
Copy link

@eguir eguir commented Nov 2, 2022

add ssh

@eguir eguir requested a review from a user November 2, 2022 15:20
@@ -0,0 +1,76 @@
import importlib.util
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regroupe test_ssh.py in test.py

@@ -0,0 +1,16 @@
#!/usr/bin/expect -f
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change suffix to .exp : it is not bash
or remove if useless

"If using a path starting with 'gs://', you must include the bucket name in it unless it"
f"If using a path starting with {TransparentPath.remote_prefix[prefix]}, you must include the bucket "
f"name in it unless it "
"is specified with bucket= or if TransparentPath already has been set to use a specified bucket"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message does not make sense if using SSH : no bucket

path: Pathlib.Path
Only relevant if the method was called from TransparentPath.__init__() : will attempts to fetch the bucket
from the path if bucket is not given
fs_kind: str Returns GCSFileSystem if 'gcs_*', LocalFilsSystem if 'local', `fsspec.implementations.local.SFTPFileSystem`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line breaks are here for a reason...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And missing a "if 'ssh'"

-------
Tuple[Union[gcsfs.GCSFileSystem, LocalFileSystem], Union[None, str], Union[None, str], Union[None, str]]
The FileSystem object, the project if on GCS else None, and the bucket if on GCS.
Tuple[Union[gcsfs.GCSFileSystem, LocalFileSystem,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned tuple is of size 3, but the doc indicates 4. Fix the doc. Plus, the typing hint and the doc do not match. Fix both.

@property
def buckets(self) -> List[str]:
if self.fs_kind == "local":
if self.fs_kind == "local" or self.fs_kind == "ssh":
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if in () blablabla

if self.fs_kind.startswith("gcs") and self.is_file():
obj = str(self).replace(TransparentPath.remote_prefix, "").replace(" ", "%20")
return f"https://storage.cloud.google.com/{obj}"
if self.fs_kind != "local" and self.fs_kind != "ssh" and self.is_file():
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not in () blablabla

@property
def download(self) -> Union[None, str]:
"""Returns a clickable link to download the file from GCS.
"""Returns a clickable link to download the file from remote.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it not implementable for ssh ? Maybe not, but it is worth checking

postfix = f";tab=objects?project={project}"
else:
return None
if self.fs_kind != "local" and self.fs_kind != "ssh":
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not replace

if self.fs_kind != "local" and self.fs_kind != "ssh":
   if self.fs_kind == "gcs":

by simply

if self.fs_kind == "gcs":

?

raise ValueError("open method needs arguments.")
thefile = args[0]
if type(thefile) == str and "gs://" in thefile:
if type(thefile) == str and ("gs://" in thefile):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mabye this condition should also handle ssh : I do not think doing open("sftp://...") actually works, you would here too need to use a transparentpath

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