run: Plumb the exit code of a command back to run#35
run: Plumb the exit code of a command back to run#35TekWizely merged 1 commit intoTekWizely:masterfrom
Conversation
TekWizely
left a comment
There was a problem hiding this comment.
Greetings! Thanks so much for taking the time to create a PR.
I think this feature idea is extremely useful !
I do have some feedback on the approach, as os.Exit(0) seems harsh in general, and since exit aborts immediately, it feels wrong to do it where it currently resides since the condition is a pass-through of the run cmd and not really an error condition in Run proper.
Have a read of my review comments and lemme know what you think.
Thanks again, and I would love to hear how you are using Run if you have time to share?
Cheers,
-TW
|
As for my use of As for what I use them for: all sorts of things, but usually general task-based jobs like "compile this project", "deploy this here", "start this in debug mode" etc. On a related note, the one thing that I think ... but, subjectively, I find the syntax a little ugly. Not sure what to suggest to improve that, though, because I guess then you get into problems like "what if I only want to run it if $condition", "what if I don't want it to exit", etc. |
65e0d01 to
13b7ffc
Compare
|
I just directly took your suggestion for the defer approach :) |
main.go
Outdated
| config.ShebangMode = len(shebangFile) > 0 && path.Base(shebangFile) != runfileDefault | ||
| } else if strings.EqualFold(os.Args[1], "version") { | ||
| showVersion() | ||
| os.Exit(0) |
There was a problem hiding this comment.
Good find ! ... But
If we're going to have the function return a value (even if its hard-coded to 0 currently), let's use it I suppose. howzabout something like:
cmdExitCode = showVersion()
return // Exit early
Seems like it should exit the program naturally and avoid the os.Exit call - What do you think?
This should probably always have had a return here vs exit. Really can't figure out why I didn't do that before ...
There was a problem hiding this comment.
Sure, that makes sense!
The only difficulty is then we need to use cmdExitCode unconditionally, not only for exit code 0. But I don't think there's a problem with doing that, so I'll push it :)
This allows failing a command if a run script fails, for example.
|
I want to keep the 0-check in the exit defer and was just expecting that version would result in exiting normally (it not exit(0)) Don't worry about changing it though - I actually have another slight mod I'm going to make, so I'll do it in a follow-up change once this is merged. |
|
Thanks so much for taking the time to contribute to Run - I'm really stoked for this feature ! @all-contributors please add @rburchell for code |
|
I've put up a pull request to add @rburchell! 🎉 |
This allows failing a command if a run script fails, for example.