Add citations and clean TODOs#1006
Conversation
Co-authored-by: OpenAI Codex <codex@openai.com>
There was a problem hiding this comment.
Pull request overview
Updates nf-core/mag’s methods reporting and repository scaffolding by removing template TODOs and adding dynamic tool citation/bibliography rendering in the MultiQC methods description.
Changes:
- Remove leftover nf-core template TODO comments from config, main workflow, and methods template.
- Implement conditional tool citation and bibliography generation (based on enabled/disabled steps) and render it into the MultiQC methods description.
- Cleanup CHANGELOG headings by removing empty sections.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/nextflow.config |
Removes placeholder TODO comments from the test config. |
subworkflows/local/utils_nfcore_mag_pipeline/main.nf |
Adds conditional tool citation + bibliography builders and enables rendering into MultiQC methods text. |
main.nf |
Removes obsolete commented template block about genome parameter values. |
assets/methods_description_template.yml |
Removes template TODO comments in the methods description template. |
CHANGELOG.md |
Removes empty section headings for consistency/cleanliness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## 5.4.2 Yellow Frog patch [2026-03-31] | ||
|
|
||
| ### `Added` |
There was a problem hiding this comment.
I personally think it's fine to leave empty sections for consistency. But up to you.
There was a problem hiding this comment.
Don't like empty sections, to me it feels that I've forgotten something hahaha
| ] | ||
|
|
||
| if (!params.skip_shortread_qc && !params.skip_clipping) { | ||
| if (params.clip_tool == 'fastp') { |
There was a problem hiding this comment.
This is very simplistic, for each of these 'sections' I personally would at least have a leading phrase, e.g. 'Preprocessing was performed by...', 'Short-read host removal was carried out with:', 'Processed reads were assembled with'.
The idea of this text is to make a good rough first draft of a methods text someone could copy paste into a methods text of a manuscript (so they do it properly). If you don't have the extra text it's sort of a bit pointless in you could just copy paste citations.md.
Simpleish example: https://github.com/nf-core/funcscan/blob/fa9db018e528ffb5149cdde928e2fa24e7c546fe/subworkflows/local/utils_nfcore_funcscan_pipeline/main.nf#L158-L267
More complex example (but my preferred method): https://github.com/nf-core/taxprofiler/blob/3c3fb68033473d723b4e9c6b090ce0f4a6ea185a/subworkflows/local/utils_nfcore_taxprofiler_pipeline/main.nf#L260-L343:
There was a problem hiding this comment.
Fair point, I'm going to make a bit more comprehensive.
jfy133
left a comment
There was a problem hiding this comment.
Unable to test myself, but I think looks good except for the one comment
| ].findAll { tool -> tool != '' } | ||
| def text_shortread_qc = "Short read preprocessing was performed with ${shortread_qc_tools.join(', ')}." | ||
|
|
||
| def text_mapping = "Read alignment was performed with Bowtie2 (Langmead and Salzberg 2012) and minimap2 (Li 2018)." |
There was a problem hiding this comment.
Given you can run with only short reads, or only long reads I don't think it makes sense to stick them in one string
| } | ||
| if (params.host_fasta || params.host_genome || !params.skip_binning || params.ancient_dna || !params.skip_ale) { | ||
| references << "<li>Langmead, B. and Salzberg, S. L. 2012 Fast gapped-read alignment with Bowtie 2. Nature methods, 9(4), p. 357–359. doi: 10.1038/nmeth.1923.</li>" | ||
| // Note: we don't have a simple way to determine if long reads are present, so we add minimap2 at the same time as Bowtie2 |
There was a problem hiding this comment.
Ah I would copy this note above.
Maybe phrasing could be changed to account for this and in a way way thatso can be deleted: 'aligning reads back to assemble was performed with bowtie2 for short reads, with minimap2 for long reads (DELETE AS APPROPRIATE)' or similar
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).