Return Result from InternalEventReader

[kdheepak]

trafficstars

I'm getting a panic for this line: https://github.com/crossterm-rs/crossterm/blob/5b19aa6d6a74438c6db4bfad4be220b400ab53da/src/event/read.rs#L38

https://github.com/kdheepak/taskwarrior-tui/runs/6213145550?check_suite_focus=true#step:8:319

When trying to call this:

let mut reader = crossterm::event::EventStream::new();

Do you think new() should return a Result type instead?

[TimonPost]

The github actions you are running crossterm in, isn't a real terminal and crossterm can not read key events from that. I forgot the exact reason of this panic, but it would not make sense for there to be any event reader as it's once initialized at the start, and if it can not initialize it does not function. Do you have tests with this waker?

[kdheepak]

I do have tests but I’ve moved the event reader creation into an if block which is disabled when running tests.

The way I think about using result types vs panics is that when there are known errors I would use result types. It would simplify my codebase if I were able to use result types for this, because I don’t believe I should be using try catch for panics here.

I’m open to suggestions on how to organize my codebase for this. Happy to close this too if you think this is the way it should be. It’s only a minor inconvenience.

[TimonPost]

I understand the concern, having a result here seems logical, while it might be a little less inconvenient for the majority of the application where this event-reader is always set. I think crossterm should strive, and it mostly is, for a zero-unwrap-policy. So I guess it's a feature request :)

[kdheepak]

That would be my preference :) Even a new function like new_safe() (name of function subject to discussion) that returned a Result type would be more than sufficient if we wanted to maintain backward compatibility + ease of use.