Skip to content

Eg.ovh#86

Open
eguir wants to merge 76 commits intomasterfrom
eg.ovh
Open

Eg.ovh#86
eguir wants to merge 76 commits intomasterfrom
eg.ovh

Conversation

@eguir
Copy link
Copy Markdown

@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
Copy Markdown

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

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

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

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

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

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

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

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

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

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":

?

Comment thread transparentpath/io/_io.py
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
Copy Markdown

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.

1 participant