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 gffread #29

Merged
merged 12 commits into from
Mar 21, 2024
Merged

Add gffread #29

merged 12 commits into from
Mar 21, 2024

Conversation

emmarousseau
Copy link
Collaborator

Description

Add gffread component.

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 gffread

  • Check the correct box. Does this PR contain:

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

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.

Hey @emmarousseau ! Thanks for creating this PR -- this already looks amazing!

I have some minor comments, see below :)

src/gffread/config.vsh.yaml Outdated Show resolved Hide resolved
Comment on lines 53 to 55
must_exist: false
description: |
Write the output records into <outfile> instead of stdout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a required argument -- the stdout produced by Viash components are only used for logging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we give a default file name instead of making it required? So no output file needs to be specified and the usage syntax stays more similar to the original gffread

Copy link
Contributor

Choose a reason for hiding this comment

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

If the component needs to return this file in order to function properly, please make it a required output file. This way, we can let Nextflow (and users) know which output files are expected and which are optional. I'll make a suggestion :)

src/gffread/config.vsh.yaml Outdated Show resolved Hide resolved
src/gffread/test_data/genome.fa Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@emmarousseau emmarousseau marked this pull request as draft March 20, 2024 17:16
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.

A suggestion (see other comment)

src/gffread/config.vsh.yaml Outdated Show resolved Hide resolved
src/gffread/script.sh Outdated Show resolved Hide resolved
@emmarousseau emmarousseau deleted the gffread-initial branch March 21, 2024 10:07
@emmarousseau emmarousseau restored the gffread-initial branch March 21, 2024 10:10
@emmarousseau emmarousseau reopened this Mar 21, 2024
@rcannood rcannood marked this pull request as ready for review March 21, 2024 11:54
@rcannood
Copy link
Contributor

Thanks @emmarousseau!

@rcannood rcannood merged commit dc62023 into viash-hub:main Mar 21, 2024
28 checks passed
@emmarousseau emmarousseau deleted the gffread-initial branch March 29, 2024 08:35
emmarousseau added a commit to emmarousseau/biobox that referenced this pull request Mar 29, 2024
* Uploading local copy

* Restructured directories and changed test data source and script

* Create var/ssoftware_versions.txt

* moved file and added quotes to all filenames

* Small formatting changes, add command tag to config file

* unset flags in script.sh and fixed argument types in config

* Refined tests

* Adjust comments

* Add changelog description, add config keyword and reorder lines

* Resolving the suggestions for PR 29

* Delete unused files

* Make output file argument required
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