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

feat: implement From<assert:cmd::Command> for process::Command #214

Closed
wants to merge 1 commit into from

Conversation

mcky
Copy link
Contributor

@mcky mcky commented Aug 8, 2024

Allows conversion with process::Command::from(asserted_cmd)

As mentioned in #212 I was having trouble serializing the value of the inner std::process::Command.

After this PR the following should be possible:

let cmd = Command::cargo_bin(env!("CARGO_PKG_NAME"))
let std_cmd = process::Command::from(cmd)

Allows conversion with `process::Command::from(asserted_cmd)`
@epage
Copy link
Contributor

epage commented Aug 9, 2024

Not sure what I think of this as its a lossy operation.

@mcky
Copy link
Contributor Author

mcky commented Aug 9, 2024

Would you be open to any API that exposed the underlying std::process::Command?

@epage
Copy link
Contributor

epage commented Aug 10, 2024

Would you be open to any API that exposed the underlying std::process::Command?

What value is using assert_cmd::Command giving you if you are then immediately wanting a std::prcoess::Command?

@mcky
Copy link
Contributor Author

mcky commented Aug 10, 2024

Many of my other tests use cargo_bin and the success/fail/stdout/stderr assertions via helpers, but for the odd interactive test I’m having to use rexpect instead, and being able to get at the internal cmd would save reworking them all.

Of course I don’t expect this merger just for my convenience, I just thought it might be generally useful to expose it in some manner (e.g. a getter or marking it public)

@epage
Copy link
Contributor

epage commented Aug 12, 2024

Considering we provide

it seems like if a std::process::Command is needed, assert_cmd shouldn't be in the way, so I'm not seeing a reason to move forward with this.

@epage epage closed this Aug 12, 2024
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