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 star solo component #62

Merged
merged 7 commits into from
Jul 29, 2024
Merged

Add star solo component #62

merged 7 commits into from
Jul 29, 2024

Conversation

rcannood
Copy link
Contributor

@rcannood rcannood commented Jun 20, 2024

Description

BREAKING CHANGES

NEW FUNCTIONALITY

Issue ticket number

Closes #xxxx

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@rcannood rcannood requested a review from DriesSchaumont July 5, 2024 12:11
Copy link
Contributor

@DriesSchaumont DriesSchaumont left a comment

Choose a reason for hiding this comment

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

Some remarks:

  • Is multiple: yes allowed by viash? If not, why was it not failing?
  • Should snakecase be used?

src/star/star_solo/config.vsh.yaml Outdated Show resolved Hide resolved
src/star/star_solo/argument_groups_solo.yaml Outdated Show resolved Hide resolved
src/star/star_solo/argument_groups_solo.yaml Outdated Show resolved Hide resolved
multiple: yes
multiple_sep: ;
- name: --soloCBstart
type: integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add min and/or max for the arguments of type: integer where applicable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be done but I don't really want to start parsing this from the help text. I propose hoping that STAR has sufficiently good validation?

src/star/star_solo/argument_groups_solo.yaml Outdated Show resolved Hide resolved
src/star/star_solo/argument_groups_solo.yaml Outdated Show resolved Hide resolved
src/star/star_solo/argument_groups_solo.yaml Outdated Show resolved Hide resolved
src/star/star_solo/argument_groups_solo.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,176 @@
#!/bin/bash

set -e
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add pipefail

src/star/star_solo/test.sh Outdated Show resolved Hide resolved
@rcannood
Copy link
Contributor Author

@DriesSchaumont I'm strongly considering dropping the star solo component and just adding the missing arguments to the main star_align_reads component. WDYT?

@DriesSchaumont
Copy link
Contributor

@DriesSchaumont I'm strongly considering dropping the star solo component and just adding the missing arguments to the main star_align_reads component. WDYT?

Yes, I agree... Its complicated already ^^

@rcannood rcannood requested a review from DriesSchaumont July 24, 2024 15:44
@rcannood
Copy link
Contributor Author

Done! @DriesSchaumont Care to re-review?

Copy link
Contributor

@DriesSchaumont DriesSchaumont left a comment

Choose a reason for hiding this comment

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

Small nitpick, feel free to merge without the change
I really like this 👍

@rcannood rcannood merged commit da414e7 into main Jul 29, 2024
3 checks passed
@rcannood rcannood deleted the add_star_solo branch July 29, 2024 07:55
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.

2 participants