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.

Sign up for GitHub

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

Jump to bottom

Seqtk subseq #85

Merged
merged 30 commits into from
Jul 18, 2024
Merged

Seqtk subseq #85

merged 30 commits into from
Jul 18, 2024

Conversation

tgaspe
Copy link
Contributor

@tgaspe tgaspe commented Jul 16, 2024 •
edited by jakubmajercik
Loading

Description

Seqtk subseq component: https://github.com/tgaspe/biobox

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

tgaspe added 11 commits July 15, 2024 15:16
@tgaspe
Config file and help.txt file
1b5d696 
Created:
- config.vsh.yaml
- help.txt
@tgaspe
Added script.sh
54b23ff 
File added:
- script.sh
@tgaspe
Created test.sh
4157f4b 
updates:
- changes to config.vsh.yaml
- created test.sh
- created some test files

Problems:
- there is some error in config file that is preventing me from running the component and testing
@tgaspe
Update on test.sh
9ca7071
@tgaspe
update
999c582
@tgaspe
Bug fixes
628a335 
- config required: false bug
@tgaspe
Update test
1337b0f
@tgaspe
Update CHANGELOG.md
5b7f207
@tgaspe
Improvement on test.sh
84f714e
@tgaspe
Added more test
88cf6d7 
I tried out different option of the command with different fasta and fastq files and different list, but the output does not seem to change.
@tgaspe
Update on tests
bd5de0a 
- got unstuck
- I need to create a docker image with the lastest version of seqtk
-
rcannood
rcannood requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @tgaspe !

I have some feedback for you -- see comments ^^

CHANGELOG.md Outdated Show resolved Hide resolved
myout.fa Outdated Show resolved Hide resolved
src/seqtk/seqtk_subseq/config.vsh.yaml Outdated Show resolved Hide resolved
src/seqtk/seqtk_subseq/test.sh Outdated Show resolved Hide resolved
tgaspe added 3 commits July 17, 2024 13:57
@tgaspe
Bug fixed
cc6746e 
- removed some test files
- fixed bug with the help of Toni
- added correct software_versions.txt to config

Still Needs:
- add one more test to strand aware
- fix tab test
@tgaspe
Update CHANGELOG.md
c782e2a
@tgaspe
Fixed Tabular test bug
06e1fe8
@rcannood
Copy link
Contributor

Could you merge main into your branch? 😊

tgaspe added 2 commits July 17, 2024 15:33
@tgaspe
Strand Aware Test
20ac10a 
- implementation of strand aware test
- change of format for reg.bed file
@tgaspe
Input validation for list file
0059ede 
- input validation for name_list parameter
@tgaspe
Copy link
Contributor Author

tgaspe commented Jul 17, 2024

Hi Robrecht and Jakub.

I think I am done.

I would love to know what you guys think!

rcannood
rcannood requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Thanks @tgaspe ! Some minor questions ^^

src/seqtk/seqtk_subseq/config.vsh.yaml Outdated Show resolved Hide resolved
src/seqtk/seqtk_subseq/config.vsh.yaml Show resolved Hide resolved
src/seqtk/seqtk_subseq/script.sh Outdated Show resolved Hide resolved
src/seqtk/seqtk_subseq/test.sh Outdated Show resolved Hide resolved
rcannood
rcannood requested changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Last comments! ^^

Also, I hijacked your PR to make some fixes to the changelog. Sorry about that.

rcannood
rcannood approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

LGTM!

@rcannood rcannood merged commit 2f9c7f7 into viash-hub:main Jul 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubmajercik jakubmajercik jakubmajercik left review comments

@rcannood rcannood rcannood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tgaspe @rcannood @jakubmajercik