Skip to content

[MAINT] Outsource checks of inputs from interface #2799

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

Closed
wants to merge 25 commits into from

Conversation

oesteban
Copy link
Contributor

Another PR removing stuff from the interface class definition. It also adds more precise exceptions.

Another PR removing stuff from the interface class definition.
@codecov-io
Copy link

codecov-io commented Nov 25, 2018

Codecov Report

Merging #2799 into master will decrease coverage by 3.34%.
The diff coverage is 95.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2799      +/-   ##
==========================================
- Coverage   67.52%   64.18%   -3.35%     
==========================================
  Files         341      340       -1     
  Lines       43312    43324      +12     
  Branches     5371     5379       +8     
==========================================
- Hits        29248    27808    -1440     
- Misses      13363    14443    +1080     
- Partials      701     1073     +372
Flag Coverage Δ
#smoketests ?
#unittests 64.18% <95.93%> (-0.77%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/fsl/utils.py 63.8% <ø> (-6.53%) ⬇️
nipype/info.py 88.23% <100%> (-5.89%) ⬇️
nipype/utils/errors.py 100% <100%> (ø)
nipype/interfaces/base/core.py 84.65% <100%> (-2.44%) ⬇️
nipype/interfaces/freesurfer/preprocess.py 64.5% <33.33%> (-1.61%) ⬇️
nipype/interfaces/base/specs.py 89.01% <95.65%> (-4.07%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 50% <0%> (-30.51%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fde422...2d5fd05. Read the comment docs.

@oesteban oesteban requested review from satra and djarecka November 26, 2018 04:39
@oesteban
Copy link
Contributor Author

There is one important change in this PR: the cmdline property of CommandLineInterface does not raise exceptions anymore, just warnings. When cmdline is not ready (e.g., because of missing mandatory inputs or conflicted xors) now it returns None and prints a warning.

The rationale is that, if the user wants to check the inputs, the appropriate way of doing it would be using the function nipype.interfaces.base.specs.check_mandatory_inputs(). After all, getting a None when calling cmdline is a strong signal of the problem, since there is no other use case where cmdline can be None. Hence it is not necessary to raise errors, which may be disruptive if used unnecessarily.

@effigies effigies added this to the 1.1.7 milestone Nov 26, 2018
@oesteban oesteban modified the milestone: 1.1.7 Nov 26, 2018
@effigies effigies modified the milestones: 1.1.7, 1.1.8 Dec 14, 2018
@effigies
Copy link
Member

Hi, just a reminder that the 1.1.8 release is targeted for January 28. Please let us know if you'd like to try to finish this up for that release.

@oesteban
Copy link
Contributor Author

This one would be good to have in 1.1.8. I'll try to resolve conflicts ASAP

@effigies
Copy link
Member

If you can wait on #2855, that should fix Travis.

@effigies effigies mentioned this pull request Jan 25, 2019
16 tasks
@effigies effigies modified the milestones: 1.1.8, future Jan 27, 2019
@effigies
Copy link
Member

This pull request is "orphaned," which means it has been deemed to be abandoned by its original author. Orphaned pull requests have not been rejected, and we hope that if a user sees one that will meet their needs with a little work, that they will fork it and open a new pull request (or, in the case of the original author, reopen the original PR).

We ask that all adopted PRs be updated to merge or rebase the current master. If you would like to adopt a PR and need help getting started, any of a number of contributors will be happy to help.

@effigies effigies closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants