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.

Sign up for GitHub

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

Jump to bottom

Add .get_kill_on_drop() method to Command #7082

Closed
czy-29 opened this issue Jan 9, 2025 · 7 comments · Fixed by #7086
Closed

Add .get_kill_on_drop() method to Command #7082

czy-29 opened this issue Jan 9, 2025 · 7 comments · Fixed by #7086
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-process Module: tokio/process

Comments

@czy-29
Copy link
Contributor

czy-29 commented Jan 9, 2025 •
edited
Loading

Is your feature request related to a problem? Please describe.
The Command of the standard library implements a series of get_* methods to obtain the options previously set by the user. Because kill_on_drop is a specific option of tokio, I think we should also add a get method to this option, which is consistent with the API pattern of the standard library.

Describe the solution you'd like

/// Returns the boolean value that was previously set by [`Command::kill_on_drop`].
///
/// Note that if you have not previously called [`Command:: kill_on_drop`], the
/// default value of `false` will be returned here.
///
/// # Examples
///
/// ```
/// use tokio::process::Command;
///
/// let mut cmd = Command::new("echo");
/// assert!(!cmd.get_kill_on_drop());
///
/// cmd.kill_on_drop(true);
/// assert!(cmd.get_kill_on_drop());
/// ```
pub fn get_kill_on_drop(&self) -> bool {
    self.kill_on_drop
}

Describe alternatives you've considered
None.

Additional context
This feature request can be regarded as a supplement to this: #7001
@czy-29 czy-29 added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Jan 9, 2025
@czy-29
Copy link
Contributor Author

czy-29 commented Jan 9, 2025

Because this feature is very simple to implement, the purpose of launching this issue is to discuss it with you first, just like last time. If you think it's OK to add, I will initiate a PR.

@Darksonn Darksonn added the M-process Module: tokio/process label Jan 10, 2025
@Darksonn
Copy link
Contributor

Hmm, Rust usually doesn't use the get_ prefix for getters.

@czy-29
Copy link
Contributor Author

czy-29 commented Jan 10, 2025

Hmm, Rust usually doesn't use the get_ prefix for getters.

Is there any better naming suggestion? It seems that this naming method also exists in the standard library, such as: https://doc.rust-lang.org/stable/std/process/struct.Command.html#method.get_current_dir

@Darksonn
Copy link
Contributor

I guess they use get_ because the usual name of a getter would just be kill_on_drop, but that method name already exists.

What do you want to use this for?

@czy-29
Copy link
Contributor Author

czy-29 commented Jan 10, 2025

What do you want to use this for?

I am abstracting a cloneable Command (but also a lossy Clone that loses the pre_exec closure) and supporting bidirectional type conversion to both std::process::Command and tokio::process::Command. I implemented this by wrapping a std::proces::Command with newtype (similar to the approach used by tokio), so when converting from tokio::process::Command to my Command, kill_on_drop will also be lost. I have stated this in my documentation, and I also hope that my unit tests can explicitly indicate that "when converting tokio::process::Command with kill_on_drop to my Command and then back to tokio::process::Command, kill_on_drop will indeed be lost". This requires tokio to have a method to obtain the value of kill_on_drop to make my wrapper type testable.

@Darksonn
Copy link
Contributor

I'm okay with adding this method.

@czy-29
Copy link
Contributor Author

czy-29 commented Jan 10, 2025

I'm okay with adding this method.

Thank you. Then I will prepare to initiate PR.

@czy-29 czy-29 mentioned this issue Jan 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-process Module: tokio/process
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

process: add Command::get_kill_on_drop() opensound-org/tokio
2 participants
@Darksonn @czy-29