Skip to content

fix: various fixes left over from refactor #142

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

Merged
merged 6 commits into from
Feb 3, 2018
Merged

Conversation

mgxd
Copy link
Member

@mgxd mgxd commented Jan 30, 2018

  • missing function imports
  • moved some variables to their proper scope
  • some pep8 fixes as well

@codecov-io
Copy link

codecov-io commented Jan 30, 2018

Codecov Report

Merging #142 into master will decrease coverage by 0.71%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #142      +/-   ##
=========================================
- Coverage   80.11%   79.4%   -0.72%     
=========================================
  Files          16      17       +1     
  Lines        1509    1534      +25     
=========================================
+ Hits         1209    1218       +9     
- Misses        300     316      +16
Impacted Files Coverage Δ
heudiconv/queue.py 25% <ø> (ø)
heudiconv/bids.py 83.44% <100%> (ø) ⬆️
heudiconv/cli/run.py 79.71% <66.66%> (+0.14%) ⬆️
heudiconv/convert.py 77.67% <75%> (-0.06%) ⬇️

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 24665ae...4fef052. Read the comment docs.

@mgxd mgxd requested a review from yarikoptic February 2, 2018 21:20
# prov information
prov_file = prefix + '_prov.ttl' if with_prov else None
if prov_file:
safe_copyfile(op.join(convertnode.base_dir,
Copy link
Member

Choose a reason for hiding this comment

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

so we need a test which runs with prov -- this clause isn't covered

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but currently i believe this option is broken with latest nipype (#108)

Copy link
Member Author

Choose a reason for hiding this comment

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

so looks like prov <=1.5.0 is fine (nipy/nipype#2240), but we should complain if user's version is higher

Copy link
Member

Choose a reason for hiding this comment

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

travis is probably fine/has an old one ;-)

@yarikoptic yarikoptic merged commit 428242c into nipy:master Feb 3, 2018
@yarikoptic yarikoptic added this to the 0.5 milestone Feb 11, 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.

3 participants