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

Add limit, offset and include_deleted to show_folder #494

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bernt-matthias
Copy link
Contributor

No description provided.

@bernt-matthias bernt-matthias marked this pull request as draft January 29, 2025 09:57
@bernt-matthias bernt-matthias force-pushed the folder-update branch 3 times, most recently from 4df08d9 to 99cd859 Compare January 29, 2025 10:24
@bernt-matthias bernt-matthias force-pushed the folder-update branch 3 times, most recently from 36073c5 to 790a7f7 Compare January 29, 2025 10:59
@bernt-matthias bernt-matthias marked this pull request as ready for review January 29, 2025 11:33
@bernt-matthias bernt-matthias changed the title Add limit and offset to show_folder Add limit, offset and include_deleted to show_folder Jan 30, 2025
bernt-matthias and others added 2 commits January 30, 2025 18:32
bioblend/_tests/TestGalaxyFolders.py Outdated Show resolved Hide resolved
bioblend/_tests/TestGalaxyFolders.py Outdated Show resolved Hide resolved
bioblend/_tests/TestGalaxyFolders.py Outdated Show resolved Hide resolved
Co-authored-by: Nicola Soranzo <[email protected]>
bernt-matthias and others added 2 commits January 30, 2025 19:42

folder_info = self.gi.folders.show_folder(self.folder["id"], contents=True)
assert len(folder_info["folder_contents"]) == 0
assert folder_info["metadata"]["total_rows"] == 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

total_rows in the metadata (which is only returned if contents=True) seems correct .. but slightly annoying that the key is different.

@bernt-matthias
Copy link
Contributor Author

Wondering if we should untangle this a bit?

  • leave show_folder as it is (maybe marking contents as deprecated)
  • add a new function show_folder_contents (or just folder_contents .. contents) that returns a generator

Using show_folder to list is not really nice ..

@bernt-matthias bernt-matthias force-pushed the folder-update branch 2 times, most recently from 237c77e to 02cd70d Compare January 31, 2025 11:23
@bernt-matthias
Copy link
Contributor Author

bernt-matthias commented Jan 31, 2025

Maybe like this: 30c4c91 simplifies the code I'm working on quite a bit:
bernt-matthias/ufz-galaxy-scripts@eeffd3a

Comment on lines +95 to +97
# if called without contents=True an item_count is reported .. lets also check this
folder_info = self.gi.folders.show_folder(self.folder["id"], include_deleted=True)
assert folder_info["item_count"] == 2
Copy link
Member

Choose a reason for hiding this comment

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

No point in testing this, since include_deleted=True is ignored you are checking the same as lines 102-104.

Suggested change
# if called without contents=True an item_count is reported .. lets also check this
folder_info = self.gi.folders.show_folder(self.folder["id"], include_deleted=True)
assert folder_info["item_count"] == 2

:rtype: dict
:return: A generator for the folder contents
"""
total_rows: Optional[int] = None
Copy link
Member

@nsoranzo nsoranzo Jan 31, 2025

Choose a reason for hiding this comment

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

Suggested change
total_rows: Optional[int] = None
total_rows = sys.maxsize

And add import sys at the top.

"include_deleted": include_deleted,
}

while total_rows is None or params["offset"] <= total_rows:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
while total_rows is None or params["offset"] <= total_rows:
while params["offset"] <= total_rows:

bioblend/galaxy/folders/__init__.py Outdated Show resolved Hide resolved
def contents(
self,
folder_id: str,
limit: int = 10,
Copy link
Member

Choose a reason for hiding this comment

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

I'd call it chunk_size in this context, as used in the rest of the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can rename this, but actually it seems that we already have a mix of limit and chunk_size (slightly more limit .. it seems).

@nsoranzo
Copy link
Member

Wondering if we should untangle this a bit?

* leave `show_folder` as it is (maybe marking `contents` as deprecated)

All BioBlend low-level wrapper methods for containers (histories, libraries, folders) currently work this way, maybe it'd be useful to improve the method annotation with overloads, like https://github.com/galaxyproject/bioblend/blob/main/bioblend/galaxy/histories/__init__.py#L307-L335

Maybe like this: 30c4c91 simplifies the code I'm working on quite a bit:
bernt-matthias/ufz-galaxy-scripts@eeffd3a

Usually this (slightly) higher-level manipulation have been implemented in BioBlend.objects , but this looks fine to me.

Co-authored-by: Nicola Soranzo <[email protected]>
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