Skip to content

Conversation

@effigies
Copy link
Contributor

@effigies effigies commented Oct 10, 2018

This is purely a style cleanup. No refactors or changes in functionality are being proposed.

First step for #10.

@codecov-io
Copy link

codecov-io commented Oct 10, 2018

Codecov Report

Merging #12 into master will decrease coverage by 0.12%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #12      +/-   ##
=========================================
- Coverage   66.22%   66.1%   -0.13%     
=========================================
  Files           7       7              
  Lines        1143    1136       -7     
  Branches      304     304              
=========================================
- Hits          757     751       -6     
+ Misses        307     306       -1     
  Partials       79      79
Flag Coverage Δ
#unittests 66.1% <72.22%> (-0.13%) ⬇️
Impacted Files Coverage Δ
pydra/engine/workers.py 92.45% <100%> (-0.28%) ⬇️
pydra/engine/state.py 58.92% <25%> (-0.73%) ⬇️
pydra/engine/newengine.py 71.42% <72.72%> (-0.04%) ⬇️
pydra/engine/auxiliary.py 74.86% <77.77%> (ø) ⬆️
pydra/engine/submitter.py 88.8% <80%> (ø) ⬆️

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 41a8d8e...b4fa03c. Read the comment docs.

@effigies
Copy link
Contributor Author

This is going to break all other PRs. @oesteban if you want to get yours in first, that's fine with me.

@rmarkello
Copy link

Thoughts on adding flake8 to the test suite as a requirement for new contributions? As you said, would be good to start off on the right foot!

@effigies
Copy link
Contributor Author

@rmarkello Yup, that would be great, but figured we should fix the issues first.

@satra
Copy link
Contributor

satra commented Oct 10, 2018

should we just ask folks to use yapf, like we do for nipype?

@rmarkello
Copy link

You can do that, too, but individual user settings in yapf may vary. I find it nice to have one of the tests fail, a la nibabel:

https://github.com/nipy/nibabel/blob/7877add916fb1aa9732d347c21b2c2f1b1d4be45/.travis.yml#L129-L131

@effigies
Copy link
Contributor Author

I agree with @rmarkello, but if we want to have a recommended YAPF configuration that we can run to make our lives easier, cool.

In any event, the consensus has been to ignore this for today, in the interest of getting things done. We can revisit after.

@oesteban
Copy link
Contributor

@effigies I'm okay with merging this first 👍

@effigies effigies changed the title STY: Flake8 cleanup [WIP] STY: Flake8 cleanup Oct 10, 2018
@effigies
Copy link
Contributor Author

Closed in favor of #15.

@effigies effigies closed this Oct 10, 2018
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.

5 participants