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

Add Cmd::inherit_stdin #98

Closed
wants to merge 5 commits into from
Closed

Conversation

cyqsimon
Copy link

@cyqsimon cyqsimon commented Dec 17, 2024

Should fix #63.

  • Implementation
  • Tests
  • Changelog

@cyqsimon
Copy link
Author

cyqsimon commented Dec 19, 2024

I've tested this patch manually and verified that it does work, but I am unsure how to automate the test. Ideas/suggestions/code welcomed.

Found a reasonable way to test it, but I ran into two problems:

  • The tests test for whether stdin is an interactive terminal, so it only works when run manually. I had to skip them for CI.
  • Due to MSRV restriction I had to bring in is-terminal for this test. I've made a note in the source to switch to std::io::IsTerminal once MSRV >= 1.70.0.

Improvement ideas welcomed.

@cyqsimon cyqsimon marked this pull request as ready for review December 19, 2024 07:19
@matklad
Copy link
Owner

matklad commented Dec 23, 2024

Thanks for the PR! I wanted to merge it, but in the end radicalized myself into re-doing most of the API. Inheriting stdin is now available at

pub fn run_interactive(&self) -> Result<()> {

@matklad matklad closed this Dec 23, 2024
@cyqsimon
Copy link
Author

but in the end radicalized myself into re-doing most of the API

Lol I get it. Happens to me too. Thanks for the good work!

@cyqsimon cyqsimon deleted the inherit-stdin branch December 24, 2024 05:19
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.

stdin broken since release 0.2.3
2 participants