-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow len(x) for select and data parameters with multiple="true"
#21343
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
base: dev
Are you sure you want to change the base?
Conversation
len(x) for select and data parameters with multiple="true"
e32d43c to
c87ef79
Compare
lib/galaxy/tool_util/xsd/galaxy.xsd
Outdated
| ### 26.0 | ||
| - The ``len`` function applied on optional data parameters without a selected dataset gives ``0``. For smaller profiles ``1`` |
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.
That seems wrong ? Nothing selected for an optional parameter (independently of multiple="true") should be None, not a list of anything, so len should just fail if we want to improve the situation.
lib/galaxy/tools/wrappers.py
Outdated
| return any(self) | ||
|
|
||
| def __len__(self) -> int: | ||
| # optional data parameters are [None] if no input is given |
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.
That seems like the thing to fix ? list|None is the type i'd assign to an optional list, not list[HDA|None]
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 would suggest [] in case nothing is in it.
The advantage would be that one can still use it in a conditional bool([]) = False if needed, but one can also just start a for loop without checking for None.
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.
Another argument for this solution might be that it is consistent to the select case.
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.
This seems so wrong. An optional multi-select without a provided value should also be None. If we merge this the only chance to get this right is yaml tools that don't run through this module :(.
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 have to admit, that I never understood the intention of this module. Where outside of the cheetah command block of XML tools are these wrappers used? Output filters for instance already seem to get None for selects already: #4221 (which was criticized there). Wondering what they get for optional data parameters :)? Funnily I also used null in the test definition, xref #19325.
So, there are definitely more arguments pro None in terms of consistency in addition to get more similar to yaml tools.
I'm just a bit afraid that we will need to touch quite a few tools ... and that the codebase needs quite some changes (i.e. more than I have time).
This seems so wrong. An optional multi-select without a provided value should also be None.
Just for my understanding. I'm wondering if a tool form can differentiate between the two case to not provide a value and provide an empy selection. Could be useful #9203, #14635.
- Message to myself: need to change XML docs for data parameters:
Unfortunately, if 0 datasets are selected the resulting value for the parameter during Cheetah templating (such > as in a command block) will effectively be a list with one None-like entity in it.
aabfa17 to
ccdd522
Compare
ccdd522 to
48cda08
Compare
For select parameters
len(str($x).slipt(","))seemed to be the best solution so far. Now we can justlen($x).Data parameters already had
len()(vialist), but returned1since it was treated as[None]for optional parameters without a value. This behavior is preserved for profile versions. From profile 26 we have[].How to test the changes?
(Select all options that apply)
License