Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change misleading message #5885

Closed
wants to merge 1 commit into from
Closed

Change misleading message #5885

wants to merge 1 commit into from

Conversation

lesleyrs
Copy link
Contributor

@lesleyrs lesleyrs commented Feb 8, 2023

Closes #5716

Simple text change copied from nvim so running commands like explorer which opens up file explorer on Windows doesn't explicitly say failed when it actually works.

New:

image

@archseer
Copy link
Member

archseer commented Feb 9, 2023

It might have not exited with exit code 1 though.

@archseer
Copy link
Member

archseer commented Feb 9, 2023

I don't see how this could be misleading, the command did fail.

@CptPotato
Copy link
Contributor

CptPotato commented Feb 9, 2023

This seems to be an issue with explorer. You can also use start on Windows to open a directory or file. The start command also returns a valid exit code (non zero if the path wasn't found). For example :sh start . will open the current directory.

On the other hand, outputting the correct exit code would be helpful in some cases.

@pascalkuthe
Copy link
Member

On the other hand, outputting the correct exit code would be helpful in some cases.

I agree, correlating exit code != 0 to failure is the right assumption most of the time so the message should still indicate a failure but actually displaying the statuscode code be helpful. For example to google the problem with whatever program you are calling. Ideally we would display stdout/stderr in a popup aswell but that can be a seperate PR.

Just changing the error message to harcode 1 is actually worse than the current error since the process might not have exited with 1 but 2 instead Command::success just checks status != 0.

I think Shell Command Failed: Status <X> where <X> is the actual status code and not just hard-coded to 1 would be a good change, for the weird edgecases that a process returns a non-zero access code even without failure this seems like an improvement.

Also @CptPotato is right that you should be using start instead of explorer directly (that will also return the right exitcode).

@lesleyrs
Copy link
Contributor Author

lesleyrs commented Feb 9, 2023

Thanks guys :sh start . gives me Command succeed unlike :sh explorer .!

And yea I was assuming failed command would always return 1 my bad.

I think Shell Command Failed: Status where is the actual status code and not just hard-coded to 1 would be a good change, for the weird edgecases that a process returns a non-zero access code even without failure this seems like an improvement.

I like this, idk how to go about doing it though. I'll close my stuff and leave it for somebody else to make a new pr.

@lesleyrs lesleyrs closed this Feb 9, 2023
@CptPotato
Copy link
Contributor

If you still want to take this on, it should be quite simple:

output.status is the program's ExitStatus which lets you retrieve the exit code using status.code().

You can display that code using string interpolation in bail!: bail!("blabla {}", exit_code). You only have to handle the case where status.code() returns None (print a message without exit code).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading message: Shell command failed
4 participants