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.

Sign up for GitHub

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

Jump to bottom

feat: Add option to modify how a query is printed #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add option to modify how a query is printed #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions snakemake_interface_storage_plugins/storage_object.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ def __init__(
self.keep_local = keep_local
self.retrieve = retrieve
self.provider = provider
self.print_query = self.provider.safe_print(self.query)
self._overwrite_local_path = None
self.__post_init__()

Expand Down Expand Up @@ -143,21 +144,21 @@ async def managed_size(self) -> int:
async with self._rate_limiter(Operation.SIZE):
return self.size()
except Exception as e:
raise WorkflowError(f"Failed to get size of {self.query}", e)
raise WorkflowError(f"Failed to get size of {self.print_query}", e)
Copy link

Choose a reason for hiding this comment

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

Use raise ... from e to distinguish the exception from errors in exception handling.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

To improve the clarity of the exception, use raise ... from e as suggested by the static analysis hint:

-            raise WorkflowError(f"Failed to get size of {self.print_query}", e)
+            raise WorkflowError(f"Failed to get size of {self.print_query}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
raise WorkflowError(f"Failed to get size of {self.print_query}", e)
raise WorkflowError(f"Failed to get size of {self.print_query}") from e
Tools
Ruff

147-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


async def managed_mtime(self) -> float:
try:
async with self._rate_limiter(Operation.MTIME):
return self.mtime()
except Exception as e:
raise WorkflowError(f"Failed to get mtime of {self.query}", e)
raise WorkflowError(f"Failed to get mtime of {self.print_query}", e)
Copy link

Choose a reason for hiding this comment

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

Use raise ... from e to distinguish the exception from errors in exception handling.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

To improve the clarity of the exception, use raise ... from e as suggested by the static analysis hint:

-            raise WorkflowError(f"Failed to get mtime of {self.print_query}", e)
+            raise WorkflowError(f"Failed to get mtime of {self.print_query}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
raise WorkflowError(f"Failed to get mtime of {self.print_query}", e)
raise WorkflowError(f"Failed to get mtime of {self.print_query}") from e
Tools
Ruff

154-154: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


async def managed_exists(self) -> bool:
try:
async with self._rate_limiter(Operation.EXISTS):
return self.exists()
except Exception as e:
raise WorkflowError(f"Failed to check existence of {self.query}", e)
raise WorkflowError(f"Failed to check existence of {self.print_query}", e)
Copy link

Choose a reason for hiding this comment

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

Use raise ... from e to distinguish the exception from errors in exception handling.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

To improve the clarity of the exception, use raise ... from e as suggested by the static analysis hint:

-            raise WorkflowError(f"Failed to check existence of {self.print_query}", e)
+            raise WorkflowError(f"Failed to check existence of {self.print_query}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
raise WorkflowError(f"Failed to check existence of {self.print_query}", e)
raise WorkflowError(f"Failed to check existence of {self.print_query}") from e
Tools
Ruff

161-161: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


async def managed_retrieve(self):
try:
Expand All @@ -173,7 +174,7 @@ async def managed_retrieve(self):
else:
os.remove(local_path)
raise WorkflowError(
f"Failed to retrieve storage object from {self.query}", e
f"Failed to retrieve storage object from {self.print_query}", e
Copy link

Choose a reason for hiding this comment

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

Use raise ... from e for consistency with the previous instances.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

For consistency with the previous instances, use raise ... from e to distinguish the exception from errors in exception handling:

-                f"Failed to retrieve storage object from {self.print_query}", e
+                f"Failed to retrieve storage object from {self.print_query}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
f"Failed to retrieve storage object from {self.print_query}", e
f"Failed to retrieve storage object from {self.print_query}") from e

)


Expand All @@ -189,14 +190,18 @@ async def managed_remove(self):
async with self._rate_limiter(Operation.REMOVE):
self.remove()
except Exception as e:
raise WorkflowError(f"Failed to remove storage object {self.query}", e)
raise WorkflowError(
f"Failed to remove storage object {self.print_query}", e
)

async def managed_store(self):
try:
async with self._rate_limiter(Operation.STORE):
self.store_object()
except Exception as e:
raise WorkflowError(f"Failed to store output in storage {self.query}", e)
raise WorkflowError(
f"Failed to store output in storage {self.print_query}", e
)


class StorageObjectGlob(StorageObjectBase):
Expand All @@ -219,4 +224,4 @@ async def managed_touch(self):
async with self._rate_limiter(Operation.TOUCH):
self.touch()
except Exception as e:
raise WorkflowError(f"Failed to touch storage object {self.query}", e)
raise WorkflowError(f"Failed to touch storage object {self.print_query}", e)
Copy link

Choose a reason for hiding this comment

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

Use raise ... from e to distinguish the exception from errors in exception handling.

The change from self.query to self.print_query in the error message is consistent with the PR objective of using the safe representation of the query in exception handling scenarios.

To improve the clarity of the exception, use raise ... from e as suggested by the static analysis hint:

-            raise WorkflowError(f"Failed to touch storage object {self.print_query}", e)
+            raise WorkflowError(f"Failed to touch storage object {self.print_query}") from e
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
raise WorkflowError(f"Failed to touch storage object {self.print_query}", e)
raise WorkflowError(f"Failed to touch storage object {self.print_query}") from e
Tools
Ruff

223-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)

8 changes: 8 additions & 0 deletions snakemake_interface_storage_plugins/storage_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,14 @@ def postprocess_query(self, query: str) -> str:
"""
return query

def safe_print(self, query: str) -> str:
"""Process the query to remove potentially sensitive information when printing.

Useful if the query is URL-like and can contain authentication tokens in the
parameters and/or usernames/passwords.
"""
return query
fgvieira marked this conversation as resolved.
Show resolved Hide resolved

@property
def is_read_write(self) -> bool:
from snakemake_interface_storage_plugins.storage_object import (
Expand Down
Loading