Skip to content
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

ENH: Allow attr_fields to exclude fields #426

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

effigies
Copy link
Contributor

@effigies effigies commented Feb 22, 2021

Acknowledgment

  • I acknowledge that this contribution will be available under the Apache 2 license.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactor

Summary

It is fairly common for calls to attr_fields to be followed by a filter for specific names that get ignored. This adds an exclusion list to the call to simplify the loops.

Checklist

  • All tests passing
  • I have added tests to cover my changes
  • I have updated documentation (if necessary)
  • My code follows the code style of this project
    (we are using black: you can pip install pre-commit,
    run pre-commit install in the pydra directory
    and black will be run automatically with each commit)

@effigies effigies force-pushed the enh/exclude_fields branch 2 times, most recently from 9cf9590 to 0b217c6 Compare February 22, 2021 03:05
@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #426 (c840917) into master (aa7231e) will decrease coverage by 0.05%.
The diff coverage is 62.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
- Coverage   82.45%   82.39%   -0.06%     
==========================================
  Files          19       19              
  Lines        3859     3846      -13     
  Branches     1054     1048       -6     
==========================================
- Hits         3182     3169      -13     
  Misses        487      487              
  Partials      190      190              
Flag Coverage Δ
unittests 82.31% <62.06%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pydra/engine/specs.py 87.21% <59.25%> (-0.29%) ⬇️
pydra/engine/boutiques.py 84.46% <100.00%> (-0.30%) ⬇️
pydra/engine/task.py 86.89% <100.00%> (-0.08%) ⬇️

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 aa7231e...c840917. Read the comment docs.

else:
raise Exception("not implemented (collect_additional_output)")
for fld in attr_fields(self, exclude_names=("return_code", "stdout", "stderr")):
if fld.type not in [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm actually not sure if we should be checking this here.. but can think about this in a different pr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make an issue and expand on your reasoning here? I agree that we should keep this PR focused.

@djarecka
Copy link
Collaborator

lgtm

@effigies effigies merged commit d730edf into nipype:master Feb 22, 2021
@effigies effigies deleted the enh/exclude_fields branch February 22, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants