TPV dry-run: mock tool has None as version

[kysrpex]

tool.version is None for the mock tool. Therefore, running TPV dry-run with rules using version helpers such as helpers.tool_version_gte causes it to crash (example here, rule causing the crash here).

In general I think it would be a good idea to have a second look on the mock objects that power the dry-run to make sure that no other similar issues are lurking in the code.

[cat-bro]

Hi @kysrpex , what is the call to dry run that is resulting in this error? The mock tool version defaults to None but should be set here https://github.com/galaxyproject/total-perspective-vortex/blob/main/tpv/commands/dryrunner.py#L34.

[kysrpex]

Hi @kysrpex , what is the call to dry run that is resulting in this error? The mock tool version defaults to None but should be set here https://github.com/galaxyproject/total-perspective-vortex/blob/main/tpv/commands/dryrunner.py#L34.

For interactive tools, such as interactive_tool_divand, '/' in tool does not hold.

[cat-bro]

It wouldn't work for interactive tools and 200+ built in tools. What version are you trying to compare in this case? And what would a sensible default for tool.version be if not None?

For any of the interactive or built in tools on Galaxy Australia version comparison would not make sense, and we are not adding this to our rules for those tools. If there is a counterexample, the dryrunner code needs updating.

[kysrpex]

what would a sensible default for tool.version be if not None

This is the big question. I have no answer neither do I have a real use-case for this. I just wanted to raise awareness that this problem may occur and if we come up with a sensible default then we can discuss it here. This is no longer an issue for us anyway.

[nuwang]

The reasonable behaviour sounds like returning False if version is None?

[cat-bro]

@nuwang I think in this case, raising an exception in the case that a tool being put up for version comparison not having a version makes sense. Anything else just returns an arbitrary wrong answer. Why not return True if version is None?

[nuwang]

@cat-bro You're right. I forgot that there were other methods like lte, eq etc.