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

Added a custom wrapper around windows commands to ignore explorer's exit status. #76

Open
wants to merge 3 commits into
base: fix-73
Choose a base branch
from

Conversation

Fundevoge
Copy link

No description provided.

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Can you point out what the difference is to the typical way of handling commands to this one? I don't see it by looking at the diff.

Thank you

@@ -249,6 +259,23 @@ impl IntoResult<io::Result<()>> for io::Result<std::process::ExitStatus> {
}
}

#[cfg(target_os = "windows")]
Copy link
Owner

Choose a reason for hiding this comment

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

This seems unused, probably because it looks like a no-op. Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

This is where the special treatment of the explorer command results from (271-274):
The exit status of the Ok variant gets ignored (272) compared to the other commands, where an exit status other than 0 indicates an error (252-256) even if the result is Ok.

src/windows.rs Outdated
.status()
}

pub fn extract(self) -> Command {
Copy link
Owner

Choose a reason for hiding this comment

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

This should be called into_command. Alternatively, From<WindowsRunCommand> can be implemented for Command.

src/windows.rs Outdated
impl Debug for WindowsRunCommand {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Start(_) => write!(f, "start"),
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be better to delegate to the inner cmd here, as every implementation prints everything it tries to execute. Windows shouldn't be different.

@Fundevoge Fundevoge marked this pull request as draft May 10, 2023 19:06
Moved extract for WindowsRunCommand to From<WindowsRunCommand>
Made Debug for WindowsRunCommand use Command's Debug
Now use into<Command>() instead of extract()
@Fundevoge Fundevoge marked this pull request as ready for review May 10, 2023 19:10
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