-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: deprecate readsfrom/writesto in favor of simply using open on AbstractCmd #6948
Conversation
(As discussed in #6945, it would be nice to eventually replace |
…tCmds, and support: open(cmd, mode) do io ... end
!success(P) && pipeline_error(P) | ||
return ret | ||
catch | ||
kill(P) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need close(io) here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that killing P
would be enough, but you're probably right that I should close(io)
too. However, this may some rearrangement to avoid calling close(io)
twice, avoid killing P
if we throw from pipeline_error(P)
, and so on.
Looks good to me, and it would be trivial to port Graphs.jl to the new interface. Display-via-pager would also be a huge win for large graphs! |
Similar to ``open(command, mode, stdio)``, but calls ``f(stream)`` | ||
on the resulting read or write stream, then closes the stream | ||
and waits for the process to complete. Returns the value returned | ||
by ``f``. | ||
|
||
.. function:: writesto(command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs to be removed.
I like it. What about standard error? Maybe |
Do you mean, add an extra parameter for |
An interesting notion is to bundle all three standard handles into some IOBundle object. Since we tend to write error messages using different function than we print things (i.e. |
Of course this is a little different since its the spawned process that is doing the writing to stderr, not us, but having a composite kind of IO object seems relevant anyway. |
It would be nice to have some In any case, that should probably wait for a separate PR. Is this PR good to merge? |
RFC: deprecate readsfrom/writesto in favor of simply using open on AbstractCmd
It seemed like the changes here were uncontroversial, and the only question is future enhancements like stderr and read/write pipes, so I went ahead and merged. |
This
sort of works but it does not seem to use |
@BobPortmann, works for me. What OS do you have? |
Mac OS X Mavericks 10.9.3
|
@BobPortmann, I just tried it on 10.9.3 with 0.3.0-prerelease+3381 (the latest nightly), and writing to |
OK. It works for me too. Not sure what I did different before. |
Would be good to document these as well, especially since they return a |
Nevermind. My helpdb.jl was out of date. |
Two comments:
|
Also, when using the method Edit: Ok, I see now that the return value is a tuple |
Maybe the form of |
I think returning the process is a good idea. I don't think it's very common to need the value of a |
On the contrary, I suspect it's extremely common to want the value of an I don't see the point of returning the |
I think the idea is to make the exit code of the process available this way (in order to know whether it failed). But I agree, I would imagine the return value of the function to be more interesting than the process/exit code in most cases. Couldn't More generally, however, I find it quite weird to have |
Ah, I believe |
I was thinking the same thing earlier--thanks for stating it in this way, @mhinsch. |
Supersedes #6945. Now you can simply do e.g.
to read from a command or e.g.
to write to a command.
The old
readsfrom(cmd)
is replaced byopen(cmd, "r")
, andwritesfrom(cmd)
is replaced byopen(cmd, "w")
.The deprecated
readsfrom
andwritesto
commands are currently used only in 5 packages: Graphs (cc: @pozorvlak, @lindahua), AWS (cc: @amitmurthy), ClusterManagers (cc: @nlhepler), ImageView (cc: @timholy), and WinRPM (cc: @ihnorton, @vtjnash).cc: @StefanKarpinski, @IainNZ, @loladiro