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

FASTQREPAIR first release #2

Merged
merged 120 commits into from
Feb 3, 2025
Merged

FASTQREPAIR first release #2

merged 120 commits into from
Feb 3, 2025

Conversation

mazzalab
Copy link
Collaborator

@mazzalab mazzalab commented Oct 28, 2024

This is the first release PR.

PR checklist

  • This comment contains a description of changes (with reason).
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@mazzalab
Copy link
Collaborator Author

Looks good to me. While waiting for CI tests to pass (see my comment in the main discussion of this PR), I have left a couple of questions.

@charles-plessy tests won't pass as per this thread: https://nfcore.slack.com/archives/C050DNB6K5H/p1738075793257549?thread_ts=1738010902.014759&cid=C050DNB6K5H
Please, tell me if I skipped any of your points.

Copy link

@atrigila atrigila left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments and for this contribution to the community! Great job!

Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Fantastic work converting all your local modules to offical nf-core ones @mazzalab !

And adding tests for local subworkflows too!

I've left a few minor comments, which should ideally be addressed (not blockers) but otherwise this looks ready to me!

Copy link
Collaborator

@tm4zza tm4zza left a comment

Choose a reason for hiding this comment

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

replied to most of @jfy133 review points

Copy link
Collaborator

@tm4zza tm4zza left a comment

Choose a reason for hiding this comment

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

fixed the remaining points.

Copy link

@LouisLeNezet LouisLeNezet left a comment

Choose a reason for hiding this comment

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

I'd like to extend James' congratulations on your fantastic migration and unit testing work.
I've just added a few little comments on things I spotted, but otherwise it's all good !

@mazzalab mazzalab merged commit 5f102ca into master Feb 3, 2025
18 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants