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 a test that a tool with id kraken2000 will not inherit parameters for kraken2 #124

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

Conversation

cat-bro
Copy link
Collaborator

@cat-bro cat-bro commented Jan 15, 2024

This is a broken test based a scenario we discovered on Galaxy Australia. A tool will inherit parameters from a tool with an ID which is the substring of the tool's ID. This is likely to have unwanted side effects.

@nuwang
Copy link
Member

nuwang commented Jan 18, 2024

My initial reaction was that this may confuse new users, although this is technically not a bug. It only happens if we forget that the tool id is meant to be a regex. As such, kraken2 would naturally match anything that kraken2000 matches. We could document the need for a $ for an exact match - the example could be corrected to include kraken2$ for exact matches.

Alternatively, we could let TPV forcibly insert the $ sign - I tried this out and it does work. The documentation changes however to "entity id is a regex, but with an EOL anchor auto-inserted at the end". I don't expect that this will break anything in the wild, even for complicated regexes, since the actual tool id always has an EOL at the end.

@bgruening @natefoo @mira-miracoli thoughts?

If we go with the 2nd option, it's technically a breaking change, although in practical terms, I doubt that anyone relies on this behaviour.

@mira-miracoli
Copy link

I don't have a strong preference, but I think it would be technically cleaner to stick to the current version and add a ⚠️ sign to the documentation, that the tool name is regex. that makes the use also more flexible in opposition to forcing the EOL character.
@sanjaysrikakulam what do you think?

@cat-bro
Copy link
Collaborator Author

cat-bro commented Jan 18, 2024

We have been sticking .* onto our keys and it is all through the examples. Regardless of whether it should work this way (I lean towards implicit ^ $, otherwise they will need to be added by us to the config for any built in tool ids) it seems like it has been regarded as being the case that the key chocolate would not match a tool with id chocolate_freckle. On the other hand substrings matching longer strings will suit @bgruening who has advocated being able to use short ids - it turns out that these worked all along.

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.

3 participants