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

New subworkflow: Fastq_fastp_trim_fastqc #3146

Merged
merged 11 commits into from
Mar 29, 2023
Merged

New subworkflow: Fastq_fastp_trim_fastqc #3146

merged 11 commits into from
Mar 29, 2023

Conversation

Joon-Klaps
Copy link
Contributor

PR checklist

Addresses second bullet from nf-core/viralrecon#371

Adds the subworkflow Fastq_trim_fastp_fastqc used in nf-core/viralrecon to nf-core/modules

  • 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
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • 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 --git-aware
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware

@Joon-Klaps Joon-Klaps added Ready for Review new subworkflow hackathon2023 Topic for the hackathon of March 2023 labels Mar 27, 2023
@matthdsm
Copy link
Contributor

I we're sticking to the guidlines, I think the name should be fastq_fastp_fastqc. Please correct me if I'm wrong 😅

matthdsm
matthdsm reviewed Mar 28, 2023
View reviewed changes
Copy link
Contributor

@matthdsm matthdsm left a comment

Choose a reason for hiding this comment

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

LGTM, but should fix the tests first

@Joon-Klaps
Copy link
Contributor Author

I we're sticking to the guidlines, I think the name should be fastq_fastp_fastqc. Please correct me if I'm wrong 😅

That's possible, no problem in changing the name. I just took what was written down on nf-core/viralrecon#371 but think the best name would then be fastq_fastqc_fastp_fastqc no?

@matthdsm
Copy link
Contributor

matthdsm commented Mar 28, 2023 •
edited
Loading

@nvnieuwk , you're the guideline specialist 😛

@nvnieuwk
Copy link
Contributor

Looks good to me except for the comments in the main.nf file (they lack path())

@Joon-Klaps
Copy link
Contributor Author

Looks good to me except for the comments in the main.nf file (they lack path())

OOh no did I do it again 🙈? I know what you mean and address it.

@nvnieuwk
Copy link
Contributor

Looks good to me except for the comments in the main.nf file (they lack path())

OOh no did I do it again 🙈? I know what you mean and address it.

No problem that's what I'm here for ;)

@Joon-Klaps
Copy link
Contributor Author

@nf-core-bot fix linting

matthdsm
matthdsm approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@matthdsm matthdsm left a comment

Choose a reason for hiding this comment

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

LGTM! Still not sure about the name though.

@Joon-Klaps
Copy link
Contributor Author

LGTM! Still not sure about the name though.

Ok, then if ur having doubts lets just change it to fastq_fastp_trim_fastqc

@Joon-Klaps Joon-Klaps changed the title New subworkflow: Fastq_trim_fastp_fastqc New subworkflow: Fastq_fastp_trim_fastqc Mar 29, 2023
@Joon-Klaps Joon-Klaps added this pull request to the merge queue Mar 29, 2023
Merged via the queue into nf-core:master with commit 4563038 Mar 29, 2023
@matthdsm
Copy link
Contributor

Awesome work 🚀 apologies for the confusion

@Joon-Klaps
Copy link
Contributor Author

LGTM! Still not sure about the name though.

Ok, then if ur having doubts lets just change it to fastq_fastp_trim_fastqc

Old name was good 'The naming convention should be of the format <file_type><operation_1><operation_n><tool_1><tool_n>' from https://nf-co.re/docs/contributing/subworkflows#naming-conventions

jvfe added a commit to jvfe/modules that referenced this pull request Mar 30, 2023
@jvfe
Merge branch 'master' into islandpath
1e2dd45 
* master:
  Ngscheckmate (nf-core#3094)
  New module Mindagap (nf-core#2860)
  Coreograph Module (nf-core#3201)
  new module wisecondorx/newref (nf-core#3212)
  Add core detection test data to config (nf-core#3199)
  Collectinsertsizemetrics (nf-core#3179)
  update appropriate label for freyja boot (nf-core#3215)
  Emit files from uncompression utilities (nf-core#3144)
  new tool: art_illumina (nf-core#3162)
  Addition of new module: Issue nf-core#3113 Survivor/filter (nf-core#3168)
  Removed -1 in CPU size specification for samtools in BWA module (nf-core#3205)
  nf-test Transition CI (nf-core#3191)
  new module: sourmash/index (nf-core#3163)
  Chopper (nf-core#3174)
  Update mapdage2 module (nf-core#3185)
  Add imaging test data in config (nf-core#3190)
  New subworkflow: Fastq_fastp_trim_fastqc (nf-core#3146)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthdsm matthdsm matthdsm approved these changes

Assignees
No one assigned
Labels
hackathon2023 Topic for the hackathon of March 2023 new subworkflow Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Joon-Klaps @matthdsm @nvnieuwk @nf-core-bot