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

Ignore process selector warnings by default #2472

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

fa2k
Copy link
Contributor

@fa2k fa2k commented Oct 16, 2023

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

This addresses issue #2161.

Changes nextflow.config in the template to set nextflow.enable.configProcessNamesValidation = false. The debug profile can then be used to re-enable these warnings.

The pull request template is updated to include a "testing with debug profile" step.

I will also send a PR for the website (docs).

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Could you also modify the documentation in CONTRIBUTING.md? Otherwise, looks good! :)
And after this PR you can also add the documentation to the website, contributing documentation is here

@@ -216,6 +217,9 @@ env {
// Capture exit codes from upstream processes when piping
process.shell = ['/bin/bash', '-euo', 'pipefail']

// Disable process selector warnings by default. Use debug profile to enable warnings.
nextflow.enable.configProcessNamesValidation = false
Copy link
Member

Choose a reason for hiding this comment

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

No need to add this param in the global scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!

Copy link
Contributor Author

@fa2k fa2k Oct 16, 2023

Choose a reason for hiding this comment

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

No need to add this param in the global scope

The default for this setting in nextflow is true. My idea for this is to set it to false by default [edit: this is why I used global scope], then return it to true if using debug. I can add it to a different scope, do you have a recommendation on that?

Copy link
Member

Choose a reason for hiding this comment

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

yes, sorry I missed that it was true by default. Your code looks good then

@fa2k
Copy link
Contributor Author

fa2k commented Oct 16, 2023

I've added the same text in CONTRIBUTING.md, so it should be ready to go when the test is done :)

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM when tests pass 🚀

@fa2k
Copy link
Contributor Author

fa2k commented Oct 16, 2023

LGTM when tests pass 🚀

Thanks! It seems to be failing due to rate limiting on some of the APIs used by the tests. pytest has passed on one platform, Python 3.11, but not the others. Is it okay to merge then?

@mirpedrol
Copy link
Member

I've re-run the failed tests, let's see if they pass now

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #2472 (65af79e) into dev (a220bcd) will decrease coverage by 0.03%.
Report is 4 commits behind head on dev.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #2472      +/-   ##
==========================================
- Coverage   70.60%   70.57%   -0.03%     
==========================================
  Files          87       87              
  Lines        9437     9421      -16     
==========================================
- Hits         6663     6649      -14     
+ Misses       2774     2772       -2     

see 7 files with indirect coverage changes

@fa2k fa2k merged commit c3ae490 into nf-core:dev Oct 16, 2023
18 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
Development

Successfully merging this pull request may close these issues.

2 participants