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 read -t #227

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: implement read -t #227

wants to merge 1 commit into from

Conversation

39555
Copy link
Contributor

@39555 39555 commented Oct 24, 2024

read -t reading stdin with timeout. Implemented with polling on unix and with a little bit insane and unsafe{} async implementation on windows. It works but work in progress. Just want to discuss different directions how we can properly implement this. Seems like non of the popular IO libraries support Windows's stdin/file IO operations because it is really a madness

@39555 39555 marked this pull request as draft October 24, 2024 14:42
Copy link

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
expand_one_string 3.42 μs 3.41 μs -0.00 μs ⚪ Unchanged
instantiate_shell 60.41 μs 59.12 μs -1.29 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 32296.22 μs 31282.56 μs -1013.66 μs ⚪ Unchanged
parse_bash_completion 2785.16 μs 2817.14 μs 31.98 μs 🟠 +1.15%
parse_sample_script 4.24 μs 4.34 μs 0.10 μs 🟠 +2.29%
run_echo_builtin_command 91.71 μs 89.54 μs -2.16 μs ⚪ Unchanged
run_one_builtin_command 108.83 μs 109.09 μs 0.27 μs ⚪ Unchanged
run_one_external_command 1973.40 μs 1989.76 μs 16.37 μs ⚪ Unchanged
run_one_external_command_directly 1045.16 μs 1025.67 μs -19.48 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/builtins/read.rs 🟠 66.27% 🟠 62.37% 🔴 -3.9%
brush-core/src/shell.rs 🟢 78.11% 🟢 78.68% 🟢 0.57%
brush-shell/src/main.rs 🟢 90.2% 🟢 90.85% 🟢 0.65%
Overall Coverage 🟢 73.53% 🟢 73.26% 🔴 -0.27%

Minimum allowed coverage is 70%, this run produced 73.26%

@reubeno
Copy link
Owner

reubeno commented Oct 24, 2024

I skimmed your changes but didn't read them too closely yet. You weren't kidding when you said the Windows side would be complicated!

I'm not against getting it working on Windows, but I think the complexity required is beyond the scope of what we should do and maintain in brush. With some exceptions, we've tried to keep away from lower-level console/terminal I/O, instead delegating those responsibilities to reedline, crossterm, nix, et al.

My vote here would be to:

  • Leverage existing abstractions in terminal/console crates to keep the built-in implementation relatively clean and platform-agnostic; where those abstractions don't do enough, abstract away any truly platform-specific details via sys.
  • Lean on existing standard crates for Unix implementation.
  • Leave the Windows side stubbed out / unimplemented for now.
  • Consider contributing/integrating your Windows-specific solution with one of the terminal/console crates in the crates ecosystem.

@reubeno reubeno added the updates_requested Pull requests with updates requested label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
updates_requested Pull requests with updates requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants