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 porechop module #914

Merged
merged 16 commits into from
Oct 28, 2021
Merged

add porechop module #914

merged 16 commits into from
Oct 28, 2021

Conversation

ggabernet
Copy link
Member

@ggabernet ggabernet commented Oct 27, 2021

Added missing porechop module by the Golden Delicious 🍏 team

PR checklist

Closes #827

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • 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 versions.yml 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

@ggabernet
Copy link
Member Author

HI our module successfully passes tests for docker and singularity, however the md5 checksums don't pass for conda, as we are getting different checksums in different systems (e.g. github actions vs locally).

modules/porechop/main.nf Outdated Show resolved Hide resolved
Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

LGTM, the conda thing is annoying, it works for docker & singularity with the checksums, right? I think we can just merge then anyways.

modules/porechop/main.nf Outdated Show resolved Hide resolved
Copy link
Contributor

@rpetit3 rpetit3 left a comment

Choose a reason for hiding this comment

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

Great work! I just have some super minor suggestions.

I think the github repo links work better then the anaconda ones since they have more info. But I'm really interested to get your thoughts on _porechop vs $options.suffix

Cheers

modules/porechop/meta.yml Outdated Show resolved Hide resolved
modules/porechop/meta.yml Outdated Show resolved Hide resolved
modules/porechop/meta.yml Show resolved Hide resolved
@ggabernet
Copy link
Member Author

Thanks for your reviews @rpetit3 and @FriederikeHanssen I've included all your review comments

@ggabernet ggabernet merged commit 0b0f87c into nf-core:master Oct 28, 2021
@d4straub d4straub mentioned this pull request Oct 28, 2021
14 tasks
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.

new module: porechop
3 participants