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

Fix 1119 #1120

Merged
merged 7 commits into from Sep 26, 2016

Conversation

Projects
None yet
5 participants
@arokem
Member

arokem commented Sep 16, 2016

Address #1119

@codecov-io

This comment has been minimized.

codecov-io commented Sep 16, 2016

Current coverage is 80.99% (diff: 100%)

Merging #1120 into master will decrease coverage by <.01%

@@             master      #1120   diff @@
==========================================
  Files           213        213          
  Lines         24032      24029     -3   
  Methods           0          0          
  Messages          0          0          
  Branches       2434       2434          
==========================================
- Hits          19465      19462     -3   
  Misses         4063       4063          
  Partials        504        504          

Powered by Codecov. Last update d0bee8c...2acb52d

@coveralls

This comment has been minimized.

coveralls commented Sep 16, 2016

Coverage Status

Coverage increased (+0.001%) to 83.086% when pulling 6ca1906 on arokem:fix-1119 into d0bee8c on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 16, 2016

Coverage Status

Coverage increased (+0.001%) to 83.086% when pulling 6ca1906 on arokem:fix-1119 into d0bee8c on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Sep 16, 2016

I've started a try_branch job on one of the failing bots, to make sure this helps:

http://nipy.bic.berkeley.edu/builders/dipy-bdist-whl-2.7/builds/44

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Sep 16, 2016

Tested it, works flawlessly.

For the XXX comments:

-get_sub_runs does needed to be overloaded in combined_workflow but thats it. If someone creates a new workflow or combined workflow, he does not need to overload it.

-From what I can see, workflow.set_sub_flows_optionals has no business there. The real implementation is in combined_workflow. Probably made sense at one point in the dev and I forgot to remove it. I think it can be removed (along with the test dipy.workflows.tests.test_workflow.test_set_sub_flows_optionals).

arokem added some commits Sep 16, 2016

@arokem

This comment has been minimized.

Member

arokem commented Sep 16, 2016

On Fri, Sep 16, 2016 at 10:11 AM, matthieudumont notifications@github.com
wrote:

Tested it, works flawlessly.

For the XXX comments:

-get_sub_runs does not needed to be overloaded in combined_workflow but
thats it. If someone creates a new workflow or combined workflow, he does
not need to overload it.

Thanks for clarifications. I am still not sure I understand. Why do we
need a class method that returns an empty list?

-From what I can see, workflow.set_sub_flows_optionals has no business
there. The real implementation is in combined_workflow. Probably made sense
at one point in the dev and I forgot to remove it. I think it can be
removed (along with the test dipy.workflows.tests.test_
workflow.test_set_sub_flows_optionals).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1120 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNiMHLgEZjC9Gl0O4Z4gvLLCVMT_zks5qqs3fgaJpZM4J_D7v
.

arokem added some commits Sep 16, 2016

@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2016

Coverage Status

Coverage decreased (-0.002%) to 83.083% when pulling 2acb52d on arokem:fix-1119 into d0bee8c on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 17, 2016

Coverage Status

Coverage decreased (-0.002%) to 83.083% when pulling 2acb52d on arokem:fix-1119 into d0bee8c on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Sep 18, 2016

This is failing coveralls because of the removal of the (unnecessary) test removed in the last commit.

@arokem

This comment has been minimized.

Member

arokem commented Sep 23, 2016

Is this GTG?

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Sep 26, 2016

Yes GTG, do I need to merge it?

@arokem

This comment has been minimized.

Member

arokem commented Sep 26, 2016

If you have push rights here, please do. Otherwise, one of the core maintainers should come along and press that green button.

@jchoude jchoude merged commit e46c7c4 into nipy:master Sep 26, 2016

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.002%) to 83.083%
Details
codecov/patch 100% of diff hit (target 80.99%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +19.00% compared to d0bee8c
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment