Code review for #52 - #68
Merged
Merged
Conversation
The 4.0.3 template renders `HEAD_REPO:` with two spaces while the prettier pin used by prek collapses it to one, so the file can never match the template and CI lint fails. Also drops the stale linting_comment.yml entry, replaced by pr-comment.yml in the 4.0.3 template.
All three still described a samtools sort subworkflow copied from the template.
Adds real pipeline-specific contribution guidelines in place of the docs/CONTRIBUTING.md placeholder.
Review point 3 from jfy133. Restores the inline comments the formatter drops.
Drops commented-out placeholders, plus outdir (set by awsfulltest) and cleanup (not a param, and unused).
All singularity shards fail with 'Failed to create user namespace: Permission denied'; latest-everything only looks green because it is continue-on-error. Apptainer 1.1.2 (action default) is non-suid and the runner image blocks unprivileged userns.
Modules publish to the topic but nothing consumed it, so the MultiQC software versions section was empty. Restores the template's collection block and drops the unused versions/mqc emits from PROFILING.
|
vinisalazar
commented
Jul 28, 2026
vinisalazar
left a comment
vinisalazar
left a comment
Collaborator
Author
There was a problem hiding this comment.
Just noting I double checked every changed file
nickp60
approved these changes
Jul 28, 2026
nickp60
left a comment
nickp60
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @vinisalazar ! A few comments below. In the future, its easier to review if you separate actual code changes from style changes. Thanks for the additional documentation and linting! Is it passing --release mode linting locally?
| emit: | ||
| profiles = ch_raw_profiles // channel: [ val(meta), [ reads ] ] - should be text files or biom | ||
| versions = ch_versions // channel: [ versions.yml ] | ||
| mqc = ch_multiqc_files |
Collaborator
There was a problem hiding this comment.
Is there a reason you are removing the mqc output?
Collaborator
Author
There was a problem hiding this comment.
It was just a Channel.empty(), should I add it back?
nf-core/test-datasets#2172 is merged, so samplesheets/test.csv and database_full.csv now resolve under pipelines_testdata_base_path. Also drops outdir and cleanup from test.config: both sat inside params, so cleanup never reached the Nextflow directive and outdir is supplied by the caller.
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
Collaborator
Author
|
@nickp60 in response to your last comment: yes, |
nf-core/test-datasets#2172 replaced KOs_sketched_scaled_1000_demo.sig.zip on the funcprofiler branch, which is where tests/nextflow.config points pipelines_testdata_base_path. The pipeline-level snapshot already carried the new md5 because it resolves the database through database.csv.
diamond 2.1.24 to 2.2.1 and seqkit 2.9.0 to 2.13.0; metaphlan stays on 4.1.1 but drops the pip/setuptools workaround and reports its version through the versions topic. gunzip and untar only gain the apptainer container-engine check. utils_nfschema_plugin renames option keys that this pipeline never sets, since it passes null for both the parameters schema and the CLI typecast.
The newer module drops the pip/setuptools dependency, leaving metaphlan on a Python 3.12 env with no distutils. Docker is unaffected.
nickp60
approved these changes
Jul 30, 2026
nickp60
left a comment
nickp60
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, thanks @vinisalazar!!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Covers:
nextflow lint -formatrun_*paramsdefault: false