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

[23.2] Updated the datatypes name for FASTK tool #18053

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

SaimMomin12
Copy link
Contributor

@SaimMomin12 SaimMomin12 commented Apr 25, 2024

Updated the datatype names of FASTK tool. This was done to make the datatypes more specific for FASTK use case.

These datatypes were not used until now, as the FASTK wrapper was not yet merged on tools-iuc.

Relevant to:

galaxyproject/tools-iuc#5965

#17265

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.

@github-actions github-actions bot added this to the 23.2 milestone Apr 25, 2024
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Sorry, we can't change them as that breaks existing tools and datasets, but you can add additional lines.

@bgruening
Copy link
Member

Can we grep in the TS if they have been used? They have been added for FASTK but FASTK was never merged and does not exist yet.

We can also use the old names, we just thought to make the names less generic.

@mvdbeek
Copy link
Member

mvdbeek commented Apr 25, 2024

No, we have no such record (and the toolshed count of clones are just fantasy numbers). I am confused though, aren't we using fastk in VGP ? If there's really very little chance we ever produced these datatypes then the change is ok.

@SaimMomin12
Copy link
Contributor Author

SaimMomin12 commented Apr 25, 2024

No, currently FASTK is not been used in VGP yet. The plan ahead is to replace the currently used tool (meryl) with FASTK

Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Ok from my side, given that fastk was never released.

@SaimMomin12
Copy link
Contributor Author

ping @mvdbeek

@mvdbeek mvdbeek merged commit ecef3ef into galaxyproject:release_23.2 Apr 29, 2024
35 of 45 checks passed
@mvdbeek
Copy link
Member

mvdbeek commented Apr 29, 2024

One approval form @bgruening is enough as long as there is no request for changes, I didn't realise this is waiting for me.

@galaxyproject galaxyproject deleted a comment from github-actions bot Apr 29, 2024
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.

3 participants