Skip to content

RF: moving more cmdline things to the cmdline folder #615

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 14 commits into from
Jun 3, 2018

Conversation

chrispycheng
Copy link
Contributor

@chrispycheng chrispycheng commented Mar 30, 2018

also added .DS_Store to the .gitignore file

  • nib-nifti-dx
  • nib-dicomfs
  • parrec2nii
  • tests for all the cmdline tools to run happily with --help
    - required a fix for PY3 in nib-dicomfs (which otherwise was not tested anyhow).
    It remains not tested on CI since would need fuse module which is not required per se

@chrispycheng chrispycheng changed the title moving more cmdline things to the cmdline folder RF: moving more cmdline things to the cmdline folder Mar 30, 2018
@codecov-io
Copy link

codecov-io commented Mar 30, 2018

Codecov Report

Merging #615 into master will decrease coverage by 1.46%.
The diff coverage is 22.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #615      +/-   ##
==========================================
- Coverage   90.31%   88.84%   -1.47%     
==========================================
  Files          87       92       +5     
  Lines       10816    11235     +419     
  Branches     1794     1839      +45     
==========================================
+ Hits         9768     9982     +214     
- Misses        719      921     +202     
- Partials      329      332       +3
Impacted Files Coverage Δ
nibabel/cmdline/parrec2nii.py 32.73% <ø> (ø)
nibabel/cmdline/dicomfs.py 18.96% <18.96%> (ø)
nibabel/cmdline/nifti_dx.py 44.44% <44.44%> (ø)
nibabel/cmdline/ls.py 17.5% <66.66%> (-1.02%) ⬇️
nibabel/imageclasses.py 100% <0%> (ø) ⬆️
nibabel/cmdline/trk2tck.py 20% <0%> (ø)
nibabel/cmdline/tck2trk.py 18.91% <0%> (ø)
nibabel/brikhead.py 97.57% <0%> (ø)

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 636d5d2...42c099b. Read the comment docs.

@coveralls
Copy link

coveralls commented Mar 30, 2018

Coverage Status

Coverage decreased (-1.6%) to 91.802% when pulling 42c099b on chrispycheng:master into 636d5d2 on nipy:master.

@yarikoptic
Copy link
Member

me still needs to figure out how to manage it to avoid freaking out crashing on import fuse during --with-doctest ...

@yarikoptic yarikoptic added this to the 2.3 milestone Apr 24, 2018
@effigies effigies mentioned this pull request May 30, 2018
30 tasks
@effigies
Copy link
Member

effigies commented Jun 1, 2018

@yarikoptic @chrispycheng Can you merge master, to make sure any errors are resulting from your code?

* origin/master: (76 commits)
  STY: Clearer conditional
  FIX: Fix for NumPy deprecation
  Better handle pathological filenames
  BF: flake8 the import
  BF(PY3): just catch all Exceptions, patch builtins module now provided in py3k
  BF: be resilient to optional module non-ImportError exceptions upon import
  better file checks - path.exists is try/except for os.stat anyways!
  Using another existing exception, and shorter message
  removing unnecessary space that nibabel checks dont like
  merging with upstream master to undo changes not ready to PR/merge
  detecting if the input is empty early on! Apparently no one needed it ;)
  DOC: rewrite first line of function docstring
  CI: Remove duplicate Travis entry [skip ci]
  mv test of failure + rm redundant tests
  COV: Omit tests, drop obsolete omissions
  add: tests for `nibabel.save`
  TEST: Delete arrayproxies, allowing Windows to close filehandles
  Revert "FIX: Give Windows time to decide files are closed"
  bf: preserve error output if not valid klass
  fix: use correct CIFTI intent names
  ...
@yarikoptic
Copy link
Member

@effigies done! and fixed up one test

@yarikoptic
Copy link
Member

any idea why I do not see travis report? ;-)

@effigies
Copy link
Member

effigies commented Jun 1, 2018

Tests are passing. Will merge this PM, barring any outcry.

@effigies
Copy link
Member

effigies commented Jun 1, 2018

I don't really know, but the pre-merge commit is passing.

@yarikoptic
Copy link
Member

as for coverage -- now, that the scripts bodies are inside the nibabel, it reflects coverage better. Main testing is done though by executing them externally, so coverage does not cover those runs. Eventually should refactor tests to just import and call within the same process to gain coverage.
I will introduce one more change to hopefully trigger the travis and appveyor, where it probably would fail due to that fuse/doctesting issue I have mentioned.

@yarikoptic
Copy link
Member

I wonder why appveyor no longer runs for this PR? Is it running for the others?

@yarikoptic
Copy link
Member

@effigies I am afraid that if you merge it as is, then it might cause appveyor and whatknows failures down the road. Better be safe than sorry ;-)

@effigies
Copy link
Member

effigies commented Jun 1, 2018

Yeah, something's happened to AppVeyor, and it's affecting the release branch, too: https://ci.appveyor.com/project/nipy/nibabel

But I don't have any permissions there, so not much I can do about it. Do you have permissions? Or do we need to bother Matthew?

@yarikoptic
Copy link
Member

hm , https://help.appveyor.com/discussions/problems/11287-the-build-phase-is-set-to-msbuild-mode-default-but-no-visual-studio-project-or-solution-files-were-found#comment_44538892 suggests that we should revoke and readd somehow... but since it also happens to release branch, it is probably not me or @chrispycheng ... ? let me try on a dummy new PR to see if somehow relates to the original author of the PR

@yarikoptic
Copy link
Member

I deauth and reauthed appveyor for myself and it started to run... so here itcomes!

@effigies
Copy link
Member

effigies commented Jun 1, 2018

@yarikoptic I assume you saw AppVeyor is failing? Windows doesn't like os.getuid() found in nib-dicomfs.

@effigies
Copy link
Member

effigies commented Jun 2, 2018

@yarikoptic My change resolved the test failure. If you want to make sure that you're okay with it, you can go ahead and merge.

@yarikoptic
Copy link
Member

@effigies thank you! I guess the battle field will show if we didn't miss anything ;-)

@yarikoptic
Copy link
Member

Doh, either merge it or I will do when get to the laptop. Silly Travis policy to not allow merging on the phone when any tests fails

@effigies effigies merged commit 2edca76 into nipy:master Jun 3, 2018
@effigies
Copy link
Member

effigies commented Jun 3, 2018

Sounds good. rel/2.3.0 is synced to the latest master. We may still see #614 in, but having enough time to test is more important. If we freeze on Monday 9am EDT, would that be enough time for you to test and get a release before OHBM?

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