Skip to content

RF: moving nib-ls to a new cmdline folder #601

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 9 commits into from
Mar 29, 2018

Conversation

chrispycheng
Copy link
Contributor

moved nib-ls to make generic use of its functions possible for a "nib-diff" function currently in progress. Shoutout to @yarikoptic

@matthew-brett
Copy link
Member

I hate to say this, but it would be good to have a few little tests in the new module, if only as a template for adding more later.

@chrispycheng
Copy link
Contributor Author

@matthew-brett can I ask what tests you're looking for? nib-ls functionality?

@matthew-brett
Copy link
Member

Sorry - I meant tests for the not-main functions that are now in the nibabel/cmdline tree.

@matthew-brett
Copy link
Member

Tests failing here - I think you need to add nibabel.cmdline to the list of packages in setup.py. You should also add nibabel.cmdline.tests (does it have an __init__.py already. There are some extra blank lines at the top of cmdline.__init__.py. Otherwise, good to go (when tests running and passing).

@chrispycheng
Copy link
Contributor Author

More tests to come for ap() and safe_get() functions

@coveralls
Copy link

coveralls commented Mar 9, 2018

Coverage Status

Coverage decreased (-3.002%) to 93.358% when pulling 6d3b735 on chrispycheng:master into 96b2bda on nipy:master.

@matthew-brett
Copy link
Member

Great, it will be good to have some more tests.

There are some PEP8 fails - see https://travis-ci.org/nipy/nibabel/jobs/351289012#L1492

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #601 into master will decrease coverage by 4.14%.
The diff coverage is 40.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #601      +/-   ##
==========================================
- Coverage   94.46%   90.31%   -4.15%     
==========================================
  Files         177       87      -90     
  Lines       24943    10810   -14133     
  Branches     2661     1793     -868     
==========================================
- Hits        23562     9763   -13799     
+ Misses        908      718     -190     
+ Partials      473      329     -144
Impacted Files Coverage Δ
nibabel/cmdline/__init__.py 100% <100%> (ø)
nibabel/cmdline/ls.py 18.51% <18.51%> (ø)
nibabel/cmdline/utils.py 68.25% <68.25%> (ø)
nibabel/filename_parser.py 91.13% <0%> (ø) ⬆️
nibabel/streamlines/array_sequence.py 100% <0%> (ø) ⬆️
nibabel/tests/test_viewers.py
nibabel/cifti2/tests/test_new_cifti2.py
nibabel/tests/test_round_trip.py
nibabel/tests/test_image_load_save.py
nibabel/tests/test_imageglobals.py
... and 105 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 96b2bda...6d3b735. Read the comment docs.

@chrispycheng
Copy link
Contributor Author

@matthew-brett Hi - if you have the time, could you please let me know what you think of the tests I've written? Thanks very much :-)

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Just a minor change to do and imho could be merged and improved upon later on

FTR codecov browser extension doesn't work for me ATM in neither firefox nor chromium

return self.test

test = TestObject()
test.test = 2
Copy link
Member

Choose a reason for hiding this comment

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

please move it into test_safe_get so its purpose is clear and it doesn't get picked up by nose etc

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done and ready to rumble

@matthew-brett
Copy link
Member

Yes, I agree - we can merge and improve. @chrispycheng - do you mind making the change Yarik suggested, and I will merge.

@yarikoptic yarikoptic merged commit 949d466 into nipy:master Mar 29, 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