-
Notifications
You must be signed in to change notification settings - Fork 193
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
added validation check for default params to nf-core lint #830
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #830 +/- ##
==========================================
- Coverage 77.72% 77.68% -0.04%
==========================================
Files 22 22
Lines 2496 2514 +18
==========================================
+ Hits 1940 1953 +13
- Misses 556 561 +5
Continue to review full report at Codecov.
|
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.
Man, I managed to come up with loads of comments for quite a small PR - sorry! 😓
Overall it looks fine and I'm sure works great, just all OCD niggles 🤓
Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>
Alright applied all the changes - thanks for the review! Hopefully I manage to make the reviews easier for you soon 😁 Apart from that there's still the question whether we should just put all of that code into the schema class? |
Looking great! Yes I think we should move it to the schema class if that's ok.. |
Sure, no problem. I also think that might be nicer :-) |
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.
So nearly there 😆 Need to run now sorry else I would have done this change and merged as it's so small. But yeah, hopefully makes sense and basically looks 💯
Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>
Awesome, thanks for putting up with my pedantic requests @KevinMenden! 😆 |
Simply added a jsonschema validation check to the
schema_lint.py
function check.So the steps are now to:
The
AssertionError
was exchanged with anException
because we capture two different errors now.Will probably make this more finegrained.
PR checklist
CHANGELOG.md
is updateddocs
is updated