Skip to content
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

RF: Make archive (de)compression use WitlessRunner #4900

Merged
merged 2 commits into from Sep 9, 2020

Conversation

mih
Copy link
Member

@mih mih commented Sep 8, 2020

Another small step.

Went for a dedicated subshell to implement the pipe, rather than an exploration how to connect two runners. Not worse than what it was before.

@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #4900 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4900      +/-   ##
==========================================
- Coverage   89.80%   89.76%   -0.04%     
==========================================
  Files         289      289              
  Lines       40659    40657       -2     
==========================================
- Hits        36512    36494      -18     
- Misses       4147     4163      +16     
Impacted Files Coverage Δ
datalad/support/archive_utils_7z.py 90.90% <100.00%> (ø)
datalad/downloaders/http.py 81.85% <0.00%> (-2.71%) ⬇️
datalad/downloaders/tests/test_http.py 87.71% <0.00%> (-2.22%) ⬇️
datalad/utils.py 82.29% <0.00%> (-0.06%) ⬇️
datalad/core/distributed/push.py 89.11% <0.00%> (-0.05%) ⬇️
datalad/support/gitrepo.py 89.40% <0.00%> (-0.03%) ⬇️
datalad/support/annexrepo.py 89.00% <0.00%> (-0.01%) ⬇️
datalad/plugin/addurls.py 99.72% <0.00%> (-0.01%) ⬇️
datalad/tests/test_utils.py 96.15% <0.00%> (ø)
datalad/cmd.py 93.03% <0.00%> (+0.37%) ⬆️

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 4f8fe6e...a656832. Read the comment docs.

else:
# fire and forget
cmd = ['7z', 'x', archive]
runner.run(cmd)
runner.run(cmd, protocol=StdOutErrCapture)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this function and the one below aren't returning the output, would it be better to use the KillOutput protocol?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. You are absolutely correct!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@mih
Copy link
Member Author

mih commented Sep 9, 2020

Thanks for the review @kyleam. I made the switch to KillOutput and will merge now. All test failures are unrelated.

@mih mih merged commit 90cf732 into datalad:master Sep 9, 2020
@mih mih deleted the rf-witlessarchives branch September 9, 2020 08:20
@bpoldrack
Copy link
Member

Hm. test_archives just started failing in master in PY3.7: https://travis-ci.org/github/datalad/datalad/builds/725525494

I'm aware that it passed here, but since it only started now, seems related to this merge.

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.

None yet

3 participants