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 fileset folder creation #427

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Rdornier
Copy link

Fix #425 by adding the fileset layer in the download folders

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

I don't know if @will-moore or @jburel had a chance to look at this yet but from my side the primary issue with the current proposal is that is it a backwards incompatible API change as the default behavior of the download_fileset API and of the omero download command is altered.

In order to enable the new functionality in a backwards-compatibility manner, could a new keyword be added to download_fileset allowing to toggle the creation of the intermediate fileset folder?

@sbesson
Copy link
Member

sbesson commented Sep 26, 2024

Thanks @Rdornier, the new option makes sense from a code review perspective and the unit tests are passing. I'll pass onto @jburel and @will-moore for inclusion in the nightly CI builds (although please note the Dundee team is also working on CI migration so this might wait a few days until the infrastructure stabilizes).

@will-moore
Copy link
Member

@Rdornier did you need to make any other changes here, such as adding insert_fileset_folder argument to the code above where this is called, and maybe to the $ omero download Fileset:1 command?

Or are you calling this function directly from elsewhere? You're using a script based on mine at
https://gist.github.com/will-moore/a9f90c97b5b6f1a0da277a5179d62c5a right?

@Rdornier
Copy link
Author

Rdornier commented Oct 1, 2024

such as adding insert_fileset_folder argument to the code above where this is called, and maybe to the $ omero download Fileset:1 command

You're right, I completly missed it. It should be done now. Thanks for your review !

You're using a script based on mine at
https://gist.github.com/will-moore/a9f90c97b5b6f1a0da277a5179d62c5a right?

Yes, initially, I used your script. But then, I changed it a bit to directly call download_fileset method. That's why I forgot to update the cli code.

@@ -61,6 +59,8 @@ def _configure(self, parser):
"OriginalFile is assumed if <object>: is omitted.")
parser.add_argument(
"filename", help="Local filename (or path for Fileset) to be saved to. '-' for stdout")
parser.add_argument(
"insert_fileset_folder", help="Adding 'Fileset_xxxx' folder in the download path", default="False")
Copy link
Member

Choose a reason for hiding this comment

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

I think here you want to use the "--insert_fileset_folder" action="store_true" instead of default="False"
Then you can check with simply if args.insert_fileset_folder:

e.g. see examples at

"--everyone", action="store_true",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conn.c.download behavior differs from java implementation
3 participants