-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add Exec2
, the "better version of Exec"
#5103
base: master
Are you sure you want to change the base?
Conversation
Error("must specify at most one working directory"); | ||
fi; | ||
dir := a; | ||
elif IsInputStream(a) then |
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.
Right now input and output stream can come at any position and in any order. Perhaps I should make this code stricter, and require that they come in a specific place (canonical places: 1. as the first two arguments; 2. after the command name; 3. as the last two arguments)
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.
While it would be a bigger change, I wonder if the input should be a record, with keys like input :=
, output :=
.. this would mean in future it would be easier to do things like add stderr support, and make it clear which output stream is stdout and which is stderr, things like that.
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 like that idea. It could then also be made backwards compatible: if there is a single argument which is a record, then use the new code; otherwise call the old code.
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.
What I don't like about it is that for simple uses, it is much more verbose. Compare: e.g.:
old code:
Exec( Concatenation("start ", winfilename ) );
this PR:
Exec2( "start", winfilename );
with a record (record entries subject to change, of course):
Exec(rec(cmd:= "start", args := [ winfilename ] );
A compromise would be to only use a rec
for the "extra" arguments like input, output, etc., like so:
Exec2( "start", winfilename, rec( input := BLAH, output := BLEH ) );
but then of course I can't use this to differentiate old and new callers.
a := Filename( DirectoriesSystemPrograms(), Concatenation( cmd, ".exe" ) ); | ||
fi; | ||
if a = fail then | ||
Error("could not locate executable for '", cmd, "'"); |
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.
Perhaps, here, just fallback to the previousExec
? Then you can rename Exec2
into Exec
since it extends the previous 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.
No, that would not be backwards compatible at all. E.g. this would change its behavior: Exec("ls", "-l", filename, ">tmpfile");
Add a new function `Exec2` which is as easy to use as `Exec` but much more powerful and less easy to misuse: - don't pass everything to a shell, thus avoiding compatibility issues due to different shells on different systems (this also has one downside: you can't use redirects like ">/dev/null") - pass arguments as separate strings, thus avoiding any issues with quoting quotes, quoting spaces, etc. - return the exit code of the executed command so that one can determine its success or failure; the return value is actually a record to allow returning other data, such as the programs output - make it very easy to override the input and output streams - by default, pass no input to the program (instead of passing anything the user might type, as `Exec` does); this can be changed by adding an input stream to the argument list - by default, capture any output of the program into a string and return it, instead of just printing the output to the console; this can be changed by adding an output stream to the argument list
(This is code I wrote some time ago, but I never found time to clean it up enough for inclusion. Due to a recent question in the GAP forum, I thought it might be a good idea to at least upload it here as a draft; perhaps someone else is interested in helping to finish it up? E.g. by coming up with a better name for the new function ;-)
Add a new function
Exec2
which is as easy to use asExec
butmuch more powerful and less easy to misuse:
Exec
does); this can be changed by adding an input stream to the argument list