-
Notifications
You must be signed in to change notification settings - Fork 196
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
Fix range type #801
Fix range type #801
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #801 +/- ##
==========================================
- Coverage 82.88% 82.79% -0.10%
==========================================
Files 13 13
Lines 2717 2702 -15
==========================================
- Hits 2252 2237 -15
Misses 465 465
Continue to review full report at Codecov.
|
nf_core/launch.py
Outdated
except ValueError: | ||
return "Must be a number" | ||
|
||
question["validate"] = validate_range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need some logic here to only use this validation if we have a min and a max? Otherwise it will just always overwrite the regular number validation function above..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, that is true. How about if I completely replace the number validation function with the range validation (or add the min, max check to the number validation, which is the same at the end)? Because it does the same as the number validation, if there is no min
or max
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup 👍🏻 Might be worth trying to add some new tests for this too? (Still not looking in depth, sorry if they are there already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a test case for ranges in test_launch.py
, if that is what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! 👍🏻
Fixes #738.
PR checklist
CHANGELOG.md
is updated