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

Don't capture so much #2460

wants to merge 6 commits into from


Copy link

commented Jul 29, 2017

This fixes #2404, covering another situation I also discovered.

My additional discovery was that stdout and stderr of hiddenobjects are also being captured, which the documentation states shouldn't be happening - I'm slightly less sure about this change, because the code quite explicitly refers to them as captured:

        elif p1 == '![':
            p0 = xonsh_call('__xonsh_subproc_captured_hiddenobject__', p2,
                            lineno=lineno, col=col)


def subproc_captured_hiddenobject(*cmds):
    """Runs a subprocess, capturing the output. Returns an instance of
    HiddenCommandPipeline representing the completed command.
    return run_subproc(cmds, captured='hiddenobject')

But I think it might a copy-paste thing from nearby code. Either way the capture is definitely causing issues, e.g. here's two invocations of fzf that select the same file:

nathan@susannah ~/projects/xonsh master $ fzf
nathan@susannah ~/projects/xonsh master $ fzf
nathan@susannah ~/projects/xonsh master $

Given that the documentation states it shouldn't be captured, and with this PR it really isn't captured anymore, I should probably also update the code to state that it's uncaptured. If you find the change agreeable let me know and I'll make those changes.


This comment has been minimized.

Copy link

commented Jul 29, 2017

Codecov Report

Merging #2460 into master will decrease coverage by 0.26%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2460      +/-   ##
- Coverage   30.86%   30.59%   -0.27%     
  Files          97       97              
  Lines       16338    16340       +2     
  Branches     3046     3047       +1     
- Hits         5042     5000      -42     
- Misses      10745    10785      +40     
- Partials      551      555       +4
Impacted Files Coverage Δ
xonsh/ 51.62% <0%> (-3.48%) ⬇️
xonsh/ 49.45% <50%> (-0.55%) ⬇️
xonsh/ 57.06% <0%> (-5.44%) ⬇️
xonsh/ 32.55% <0%> (-2.33%) ⬇️
xonsh/ 32.7% <0%> (-0.47%) ⬇️
xonsh/ 29.09% <0%> (+9.83%) ⬆️

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 5cb3e8d...cede0e5. Read the comment docs.


This comment has been minimized.

Copy link

commented Nov 6, 2017

Hey @nathan-hoad -- sorry for the long delay in getting to this. The changes sound reasonable to me, and yes, the uncaptured commands should probably be uncaptured. Do you want to go ahead and update the code to reflect that?


This comment has been minimized.

Copy link

commented Nov 8, 2017

Hello All, Thanks for the PR @nathan-hoad! So I think that there might be some confusion over terminology here. un/capturing only refers to whether the output is piped to stdout and stderr. The distinction between $[cmd] and ![cmd] -- even though both will stream stdout -- is that:

  • $[cmd] will always return None
  • ![cmd] will always return a rich HiddenCommandPipeline instance, which even let's you inspect the output of the command you have just run.

For example on master, we see what I think is the correct behaviour:

scopatz@artemis ~ $ $[echo hello] is None
scopatz@artemis ~ $ ![echo hello].out

On this branch, we no longer have access to the full output after the command is run:

scopatz@artemis ~ $ $[echo hello] is None
scopatz@artemis ~ $ ![echo hello].out

Right now, all subprocess commands are automatically wrapped in ![cmd], rather than $[cmd]. If this is problematic for some reason, maybe we should allow folks to set which way they want uncaptured commands wrapped (either with an object or without). I'd welcome a PR to this effect. I hope that this helps!

PS super sorry about taking so long to get to this.


This comment has been minimized.

Copy link

commented Jul 20, 2018

I'm going to close this out in light of @scopatz' comments, but please ping here if anyone wants to continue this discussion and/or change the direction of this PR.

@gforsyth gforsyth closed this Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
4 participants
You can’t perform that action at this time.