-
Notifications
You must be signed in to change notification settings - Fork 59
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
REF: More detailed runtime checks for input spec #627
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #627 +/- ##
==========================================
- Coverage 81.76% 81.70% -0.06%
==========================================
Files 20 20
Lines 4392 4390 -2
Branches 1264 0 -1264
==========================================
- Hits 3591 3587 -4
- Misses 797 803 +6
+ Partials 4 0 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Deleted some bad suggestions. Will do normal review so you don't see the noise. |
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.
The logic looks good. I have some suggestions that I put in a commit that I think make it a little easier to follow (2eb09b7).
First we check whether we need to look at alternatives or requires. Then we look at alternatives. Then we look at requires. The behavior doesn't change (except the specific text of an error message). Your logic looks good overall.
Note that I changed if x and any(x.values())
, to if any(x.values())
, as that will return False
on an empty sequence. Same with not all(x.values())
.
dc22246
to
6075df9
Compare
I like them. I added them on your behalf in a follow-up commit. |
6075df9
to
200dd4b
Compare
2d75f3c
to
32432c3
Compare
Whilst working on #619, I had a hard time figuring out how runtime checks were working in the input spec.
I took this opportunity to break the checks down into more digestible chunks, with intermediate variables and more meaningful names. In addition, we are now getting the list of alternative fields to set in case a mandatory argument is unset but declare other fields in
xor
, instead of just reporting the field as mandatory but unset.On a more general note, I would have expected to find these checks in
ShellSpec
, since the specific metadata we are manipulating only make sense in this context. Food for thoughts for a wider refactoring maybe.Types of changes
Checklist