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

run: Plumb the exit code of a command back to run #35

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

rburchell
Copy link
Contributor

This allows failing a command if a run script fails, for example.

Copy link
Owner

@TekWizely TekWizely left a comment

Choose a reason for hiding this comment

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

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

main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@rburchell
Copy link
Contributor Author

As for my use of run, well... I have a set of shell scripts that do more or less the same thing, although in a less elegant way. They also lack a lot of features like command line argument handling, so run looks like a good fit to get something with more capabilities without the (painful) effort of continuing to extend a non-portable shell script harness.

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 run could use is an easier way to mark steps as dependent. Like, at the moment, I'm doing this:

##
# Runs unit tests
test:
    "${RUN}" -r "${RUNFILE}" build --builddir="build" --device="desktop" || exit 1

... 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.

@rburchell
Copy link
Contributor Author

I just directly took your suggestion for the defer approach :)

main.go Outdated
@@ -105,6 +117,7 @@ func main() {
config.ShebangMode = len(shebangFile) > 0 && path.Base(shebangFile) != runfileDefault
} else if strings.EqualFold(os.Args[1], "version") {
showVersion()
os.Exit(0)
Copy link
Owner

Choose a reason for hiding this comment

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

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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
@TekWizely
Copy link
Owner

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.

@TekWizely TekWizely merged commit 0e47b84 into TekWizely:master Sep 7, 2021
@TekWizely
Copy link
Owner

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

@allcontributors
Copy link
Contributor

@TekWizely

I've put up a pull request to add @rburchell! 🎉

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

2 participants