-
Notifications
You must be signed in to change notification settings - Fork 32
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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?
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). |
@Rdornier did you need to make any other changes here, such as adding Or are you calling this function directly from elsewhere? You're using a script based on mine at |
You're right, I completly missed it. It should be done now. Thanks for your review !
Yes, initially, I used your script. But then, I changed it a bit to directly call |
src/omero/plugins/download.py
Outdated
@@ -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") |
There was a problem hiding this comment.
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
omero-py/src/omero/plugins/admin.py
Line 269 in 4fd72ef
"--everyone", action="store_true", |
Fix #425 by adding the fileset layer in the download folders