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

[24.2] Add linters for datatypes #17600

Merged
merged 15 commits into from
Nov 22, 2024

Conversation

bernt-matthias
Copy link
Contributor

  • tools should only use available datatypes in inputs, outputs and tests
  • discourage the use of custom datatypes_conf.xml

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@mvdbeek mvdbeek removed this from the 24.1 milestone May 14, 2024
@bernt-matthias
Copy link
Contributor Author

Is this what you thought about @mvdbeek and @nsoranzo? Then I would be happy to get this in.

@bernt-matthias bernt-matthias requested a review from a team October 22, 2024 13:17
@bernt-matthias
Copy link
Contributor Author

Would be interested to know why this did not make it into 24.2? It just feels very unsatisfying to open this months ago, spend time to update the branch multiple times, and make it ready prior to freeze, and even requesting review ahead of time to get this of the list before the busy times during the release.

@jdavcs jdavcs changed the base branch from dev to release_24.2 November 22, 2024 15:35
@github-actions github-actions bot changed the title Add linters for datatypes [24.2] Add linters for datatypes Nov 22, 2024
@jdavcs
Copy link
Member

jdavcs commented Nov 22, 2024

Would be interested to know why this did not make it into 24.2? It just feels very unsatisfying to open this months ago, spend time to update the branch multiple times, and make it ready prior to freeze, and even requesting review ahead of time to get this of the list before the busy times during the release.

I am sorry - this was obviously an oversight. Merging now.

@jdavcs jdavcs merged commit 6bc9940 into galaxyproject:release_24.2 Nov 22, 2024
55 of 56 checks passed
@bernt-matthias bernt-matthias deleted the datatype-lint branch November 22, 2024 15:37
Copy link

This PR was merged without a "kind/" label, please correct.

@jdavcs jdavcs modified the milestones: 25.0, 24.2 Nov 22, 2024
from galaxy.tool_util.lint import LintContext
from galaxy.tool_util.parser import ToolSource

DATATYPES_CONF = os.path.join(os.path.dirname(__file__), "datatypes_conf.xml.sample")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like:

from pkg_resources import resource_filename
DATATYPES_CONF = resource_filename('galaxy.config', 'sample/datatypes_conf.xml.sample')

would again require config being installed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pkg_resources is deprecated, but using galaxy.util.resource_path would be a minor improvement here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to rely on importlib.resources, wouldn't this mean that we rely on config again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've created a copy by including datatypes_conf.xml.sample" in the manifest, so the galaxy.tool_util.linters package should have that file.

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

Successfully merging this pull request may close these issues.

4 participants