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

2451 schema build update defaults #2479

Merged
merged 15 commits into from
Dec 14, 2023

Conversation

awgymer
Copy link
Contributor

@awgymer awgymer commented Oct 17, 2023

Fixes #2451

Implemented the option (via questionary) to update the schema defaults where the default in the pipeline appears to have changed.

A similar but reverse issue is found in #1954
Perhaps there is some scope here to issue warnings to assist in that?

Also implemented some additional code to ensure that non-zero False-y defaults are no longer written by python (current behaviour relies on web-builder to handle this but that is not used by all)

Considerations:

  • Should determine some suitable tests for this new behaviour
  • Do we need to allow params to be excluded (in e.g. .nf-core.yml) so that known divergent defaults can be safely ignored?

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

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (960bff5) 74.98% compared to head (2c9ccec) 74.93%.
Report is 4 commits behind head on dev.

Files Patch % Lines
nf_core/schema.py 51.61% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2479      +/-   ##
==========================================
- Coverage   74.98%   74.93%   -0.06%     
==========================================
  Files          85       85              
  Lines        9327     9356      +29     
==========================================
+ Hits         6994     7011      +17     
- Misses       2333     2345      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 thank you!!
But the new mypy linting is failing, so we should have a look before merging :)

@mashehu
Copy link
Contributor

mashehu commented Oct 23, 2023

LGTM thank you!! But the new mypy linting is failing, so we should have a look before merging :)

These are all not yet typed files, so it is a case related to this #2480 and can be ignored, I think

@mirpedrol
Copy link
Member

I see! OK for me to merge then

@ewels ewels added this to the 2.11 milestone Dec 8, 2023
@mirpedrol
Copy link
Member

@nf-core-bot fix linting please!

@mirpedrol mirpedrol merged commit e131764 into nf-core:dev Dec 14, 2023
20 checks passed
@awgymer awgymer deleted the 2451-schema-build-update-defaults branch December 14, 2023 13:51
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.

5 participants