nipype / pydra

Pydra Dataflow Engine
https://nipype.github.io/pydra/
Other
120 stars 59 forks source link

REF: More detailed runtime checks for input spec #627

Closed ghisvail closed 1 year ago

ghisvail commented 1 year ago

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

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.06 :warning:

Comparison is base (374cdec) 81.76% compared to head (082e600) 81.70%.

:exclamation: Current head 082e600 differs from pull request most recent head 32432c3. Consider uploading reports for the commit 32432c3 to get more accurate results

Additional details and impacted files ```diff @@ 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 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `81.70% <100.00%> (-0.06%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/nipype/pydra/pull/627?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [pydra/engine/specs.py](https://codecov.io/gh/nipype/pydra/pull/627?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL3NwZWNzLnB5) | `94.74% <100.00%> (-0.03%)` | :arrow_down: | ... and [4 files with indirect coverage changes](https://codecov.io/gh/nipype/pydra/pull/627/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

effigies commented 1 year ago

Deleted some bad suggestions. Will do normal review so you don't see the noise.

ghisvail commented 1 year ago

The logic looks good. I have some suggestions that I put in a commit that I think make it a little easier to follow (https://github.com/nipype/pydra/commit/2eb09b77e010525579cdc6324b929a260c0addb3).

I like them. I added them on your behalf in a follow-up commit.