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

Treat non-zero exit code as error by default #597

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

niik
Copy link
Member

@niik niik commented Oct 23, 2024

Note: this would be a breaking change

The default behavior of Node's execFile is to treat any exitCode other than 0 as an error. Since dugite's inception we've chosen to ignore these errors and pretend everything was fine, leaving it up to the caller to inspect the exitCode property to detect if the command succeeded or not.

GitHub Desktop worked around this by throwing a new error of its own if the exit code was not part of the 'successExitCodes` set.

This change would copy that approach into Dugite and I'm exploring what that would look like in this PR. I'm not necessarily committed to the change.

Why the change?

Partially to align closer with execFile and thereby making it easier for callers to opt out of using our exec function and rolling their own instead. Partially because it feels weird to put it on the caller to always inspect exitCode instead of notifying them when something unexpected occurred.

Why include ignoreExitCodes in dugite?

So I had wanted to avoid this but it turns out the change I made in #582 made it awkward for callers to catch this specific exception and then convert it to an IGitResult on their own as it'd break the nice type matching on IGitStringResult vs IGitBufferResult.

@sergiou87 @tidy-dev We can start with the question of whether y'all think this change makes sense before we dive too deep into code review.

@sergiou87
Copy link
Member

I agree with your reasoning, so I wouldn't be against it. I can't tell if this actually will make our lives easier or more complicated, though 😂 So I don't have a strong opinion other than "this makes sense and makes our API more idiomatic" 🤔

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.

2 participants