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

XSD: the tool id attribute allows any string as per the schema definition #11196

Closed
davelopez opened this issue Jan 21, 2021 · 7 comments
Closed

Comments

@davelopez
Copy link
Contributor

The current definition in the XSD file for the tool id attribute allows any string while the attribute documentation states:

should be lowercase and contain only letters, numbers, and underscores.

and the IUC best practices recommendations states:

  • Tool IDs should contain only [a-z0-9_-].
  • Multiple words should be separated by underscore or dashes

I propose adding a <xs:simpleType> with a pattern restriction that complies with these requirements. Or should the schema not enforce this rule to validate the XML document?

@bgruening
Copy link
Member

Plus one for enforcing.

@mvdbeek
Copy link
Member

mvdbeek commented Jan 21, 2021

As long as this is just a warning in the linter I think that's fine ... keep in mind that changing tool ids break tool lineages.

@davelopez
Copy link
Contributor Author

Adding this restriction and providing an invalid id will probably result in planemo lint displaying Invalid XML found in file with the corresponding error message and the Galaxy Tools Extension showing an issue in the problems tab similar to this:

image

As far as I know, it should not break any existing tool id, since this is only used when linting tools, but probably will mark planemo lint as failed in the CI when making a PR against a tool repository.
Is this behavior undesired?

@mvdbeek
Copy link
Member

mvdbeek commented Jan 21, 2021

I think so, since that means we can't update the tool for fixes.

@davelopez
Copy link
Contributor Author

Oops... Understand! 😅
Then do not add the restriction to the XSD and probably include an additional custom validation to the extension and/or planemo linter that will check this pattern and treat it only as a Warning.

I will close this then :)

@bgruening
Copy link
Member

I think so, since that means we can't update the tool for fixes.

I think we should lint tools like this, to avoid people adding more "tools with spaces in IDs". The old tools are then failing, that is another problem we should fix elsewhere, e.g. skipping lints in shed.yml ... but the correct thing is to warn users about a "wrong" ID, isn't it?

@mvdbeek
Copy link
Member

mvdbeek commented Jan 21, 2021

I think David suggested the right thing here. The XSD linting doesn't allow this amount of flexibility.

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

No branches or pull requests

3 participants