ENH: ReportCapableInterface mix-in/base interface #2560
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2560 +/- ##
==========================================
- Coverage 67.57% 67.56% -0.02%
==========================================
Files 333 335 +2
Lines 42547 42589 +42
Branches 5266 5269 +3
==========================================
+ Hits 28750 28774 +24
- Misses 13124 13142 +18
Partials 673 673
Continue to review full report at Codecov.
|
|
@effigies is this targeting |
|
No. For my part the only things to try to get in for 1.0.3 are any fixes needed to pass all the tests and avoid distribution errors. |
| runtime = self._run_interface(runtime) | ||
| runtime = self._post_run_hook(runtime) |
There was a problem hiding this comment.
@satra Are you comfortable with this change to BaseInterface?
There was a problem hiding this comment.
for now yes - we may have to add some prov stuff when i refactor that piece.
|
|
||
|
|
||
| class ReportCapableInputSpec(BaseInterfaceInputSpec): | ||
| generate_report = traits.Bool(False, usedefault=True, |
There was a problem hiding this comment.
As for terminal_output and ignore_exception, this should not be an input. Should be a state variable of the instance.
| if not self.inputs.generate_report: | ||
| return runtime | ||
|
|
||
| self._out_report = os.path.abspath(self.inputs.out_report) |
There was a problem hiding this comment.
current working directory might have changed at this point :(
There was a problem hiding this comment.
I see three options:
chdir(runtime.cwd)between each step - ugly, but forgivingRuntimeErrorifos.getcwd() != runtime.cwdafter one of these steps is run- Reject the idea of cwd assurances altogether and resolve this path relative to
runtime.cwd
There was a problem hiding this comment.
Actually, I'm going to do 3 for now. @satra, let me know if you think we should do any work to either mitigate or error out if _*_run_hook and _run_interface change the CWD on us.
|
I was able to use this PR to refactor reporting interfaces in nipreps/niworkflows#238, so this should be good to go. |
Related (closes?): #2553
Changes proposed in this pull request
ReportCapableInterfaceas a mix-in that is also written to work as a base class as well.This factors out what seemed to be the essential parts of the niworkflows
ReportCapableInterface.Differences:
_post_run_hookis now the driver of this functionality, and classes using this mix-in should tail-callreturn super(..., self)._post_run_hook(runtime)._generate_error_reportwas removed. I don't think this can actually be hit in niworkflows, asCommandLineinterfaces that had bad return values raised exceptions.