Skip to content

Implement Deref to be able to use std::process:Command methods#9

Merged
sharkdp merged 2 commits intosharkdp:masterfrom
themkat:get_program
Sep 4, 2022
Merged

Implement Deref to be able to use std::process:Command methods#9
sharkdp merged 2 commits intosharkdp:masterfrom
themkat:get_program

Conversation

@themkat
Copy link
Contributor

@themkat themkat commented Aug 20, 2022

I find this useful in std::process:Command. Useful for possible solutions to something like (writing custom formatting methods for it):
sharkdp/fd#1083

...and off course other places where argmax is used and we want to print the program name for whatever reason. It will include the entire path given to std::process:Command::new.

Feel free to ignore/close if this is something that has already been considered, or if you just think it is stupid 🙂

@tavianator
Copy link
Contributor

I think I tried this in #3 but backed it out to avoid bumping the MSRV.

Maybe we could just impl Deref and get all the methods for free?

@themkat
Copy link
Contributor Author

themkat commented Aug 20, 2022

Maybe we could just impl Deref and get all the methods for free?

I actually considered that, but thought there was a strategy behind not exposing process::Command. Will push a new commit where I implement Deref, as I also think it is the best solution here 🙂 Kept the other methods that was added in your PR for now, but they can probably be removed.

@themkat themkat changed the title Add get_program method to be able to fetch the name of the program Implement Deref to be able to use std::process:Command methods Aug 21, 2022
@themkat
Copy link
Contributor Author

themkat commented Aug 21, 2022

Removed the usage of get_program in the test now, as it seems to have caused the minimum supported rust version error.

@sharkdp
Copy link
Owner

sharkdp commented Aug 22, 2022

The MIN_SUPPORTED_RUST_VERSION is also quite low, so I'd be okay with bumping it. But if we agree that we don't really need a test for this, I'm also fine with this Deref implementation that simply adapts to the currently used std::process::Command API.

Kept the other methods that was added in your PR for now, but they can probably be removed.

I'd be in favor of that! How does it change the documentation output? Would users still see the full std::process::Command API?

@themkat
Copy link
Contributor Author

themkat commented Aug 22, 2022

I'd be in favor of that! How does it change the documentation output? Would users still see the full std::process::Command API?

My bad. It seems like most of those methods require us to implement DerefMut as well (because of the mutable self reference). Tried it quickly, and I think the documentation is a bit harder to read (as we get quite a lot of new methods in the Deref methods section of the doc). I'd say we keep it like this (with only Deref) for now. With that change we get get_program, get_args, get_env and a few other useful methods.

@tavianator
Copy link
Contributor

We shouldn't implement DerefMut anyway as then users can bypass the command size accounting

@sharkdp
Copy link
Owner

sharkdp commented Sep 4, 2022

Ok, thank you!

@sharkdp sharkdp merged commit e16ef14 into sharkdp:master Sep 4, 2022
@themkat themkat deleted the get_program branch September 4, 2022 13:23
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.

3 participants