reubeno / brush

bash/POSIX-compatible shell implemented in Rust
MIT License
20 stars 4 forks source link

feat: implement `read -t` #227

Open 39555 opened 5 days ago

39555 commented 5 days ago

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

github-actions[bot] commented 5 days ago

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 commented 5 days ago

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: