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

Try adding nextflow strict #461

Merged
merged 16 commits into from
Apr 4, 2024
Merged

Try adding nextflow strict #461

merged 16 commits into from
Apr 4, 2024

Conversation

jfy133
Copy link
Member

@jfy133 jfy133 commented Mar 21, 2024

Experimentation following: https://nfcore.slack.com/archives/C043UU89KKQ/p1709835715768389

PR checklist

  • 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 pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/taxprofiler branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core 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).

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 21, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 2ebce4c

+| ✅ 193 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in ci.yml: You can customise CI pipeline run tests as required
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-04-04 10:48:31

@jfy133
Copy link
Member Author

jfy133 commented Mar 22, 2024

OK I'm getting some weird variation which I'm still investigating, but for now...

@jfy133
Copy link
Member Author

jfy133 commented Mar 22, 2024

OK it magically has disappeared... we will have to monitor this... basically some of the db_params were being emitted as null and so split() would stop working... I replicated it a couple of tiemes ion my laptop but now it doesn't happen.

@LilyAnderssonLee
Copy link
Contributor

I am also confused by this 'null' error. Do you know if this issue occurs for anyone else?

LilyAnderssonLee
LilyAnderssonLee approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@LilyAnderssonLee LilyAnderssonLee left a comment

Choose a reason for hiding this comment

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

Well done! I tested it locally and it performed exactly as expected. LGTM!

@jfy133
Fix occasional split errors, due to db_params replacement not actuall…
4dc0900 
…y being assigned to a channel, ensure new metamap made on db_param cleanup
@jfy133 jfy133 mentioned this pull request Mar 28, 2024
Merged
3 tasks
@jfy133 @Midnighter
Update workflows/taxprofiler.nf
630a25c 
Co-authored-by: Moritz E. Beber <[email protected]>
@LilyAnderssonLee
Copy link
Contributor

LilyAnderssonLee commented Apr 2, 2024 •
edited
Loading

Not sure why there is a krakentools folder in the results, and also there is an untar folder. Uncertain about when this error was introduced. It might be from the nf-validation step PR.

Here's the code to reproduce the result: nextflow run main.nf -profile singularity,test --outdir results.

@jfy133
Copy link
Member Author

jfy133 commented Apr 4, 2024

@LilyAnderssonLee you're right it came with the template, I guess there is now a default folder name for all modules. I'll have a look to see if they are relevant :)

@jfy133
Add option to save uncompressed databases, turn
1b4c5f0 
off Kraken2 to krona conversion output file
publishing
jfy133
jfy133 commented Apr 4, 2024
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@jfy133
Copy link
Member Author

jfy133 commented Apr 4, 2024

@nf-core-bot fix linting

LilyAnderssonLee
LilyAnderssonLee reviewed Apr 4, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@jfy133 @LilyAnderssonLee
Update CHANGELOG.md
f894a4b 
Co-authored-by: Lili Andersson-Li <[email protected]>
@jfy133 @LilyAnderssonLee
Update nextflow_schema.json
2ebce4c 
Co-authored-by: Lili Andersson-Li <[email protected]>
@LilyAnderssonLee
Copy link
Contributor

Tested locally and worked well. Good job!

@jfy133 jfy133 merged commit 593c013 into dev Apr 4, 2024
24 checks passed
@jfy133 jfy133 deleted the nextflow-enable-strict branch April 4, 2024 13:50
@jfy133 jfy133 mentioned this pull request Apr 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LilyAnderssonLee LilyAnderssonLee LilyAnderssonLee approved these changes

@sofstam sofstam Awaiting requested review from sofstam

@Midnighter Midnighter Awaiting requested review from Midnighter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jfy133 @LilyAnderssonLee @Midnighter @nf-core-bot