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

new module: rasusa #413

Merged
merged 9 commits into from
Apr 9, 2021
Merged

new module: rasusa #413

merged 9 commits into from
Apr 9, 2021

Conversation

thanhleviet
Copy link
Contributor

@thanhleviet thanhleviet commented Apr 7, 2021

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • [x ] If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the <SOFTWARE>.version.txt file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Looks great @thanhleviet ! Few changes. Test weren't triggered because this file wasnt updated with the module.

Did you use nf-core modules create or manually create the module? The former would have automatically updated that file.

software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/meta.yml Outdated Show resolved Hide resolved
tests/software/rasusa/main.nf Outdated Show resolved Hide resolved
tests/software/rasusa/main.nf Outdated Show resolved Hide resolved
tests/software/rasusa/main.nf Outdated Show resolved Hide resolved
tests/software/rasusa/test.yml Outdated Show resolved Hide resolved
@drpatelh drpatelh added the new module Adding a new module label Apr 8, 2021
@thanhleviet
Copy link
Contributor Author

Looks great @thanhleviet ! Few changes. Test weren't triggered because this file wasnt updated with the module.

Did you use nf-core modules create or manually create the module? The former would have automatically updated that file.

yes I used nf-core command.


script:
def software = getSoftwareName(task.process)
def prefix = options.suffix ? "${meta.id}${options.suffix}_${depth_cutoff}X" : "${meta.id}_${depth_cutoff}X"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def prefix = options.suffix ? "${meta.id}${options.suffix}_${depth_cutoff}X" : "${meta.id}_${depth_cutoff}X"
def prefix = options.suffix ? "${meta.id}.${options.suffix}" : "${meta.id}"

Copy link
Member

Choose a reason for hiding this comment

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

The suffix should come in via options.suffix to keep it completely flexible for developers to name their files however they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, how can I assign depth_cutoff to suffix in a flexible way so that we don't need to define this twice?

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Looks awesome @thanhleviet ! Almost there. Just a few little changes left.


script:
def software = getSoftwareName(task.process)
def prefix = options.suffix ? "${meta.id}${options.suffix}_${depth_cutoff}X" : "${meta.id}_${depth_cutoff}X"
Copy link
Member

Choose a reason for hiding this comment

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

The suffix should come in via options.suffix to keep it completely flexible for developers to name their files however they want.

Comment on lines 22 to 24
tuple val(meta), path(reads)
val depth_cutoff
val genome_size
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tuple val(meta), path(reads)
val depth_cutoff
val genome_size
tuple val(meta), path(reads)
val depth_cutoff
val genome_size

software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/meta.yml Show resolved Hide resolved
tests/software/rasusa/main.nf Outdated Show resolved Hide resolved
tests/software/rasusa/main.nf Outdated Show resolved Hide resolved
tests/software/rasusa/main.nf Outdated Show resolved Hide resolved
software/rasusa/main.nf Outdated Show resolved Hide resolved
Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

👌🏽

@drpatelh drpatelh merged commit 5a59e61 into nf-core:master Apr 9, 2021
@thanhleviet thanhleviet deleted the rasusa branch April 12, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new module Adding a new module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants