-
Notifications
You must be signed in to change notification settings - Fork 51
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
added y/n prompt #144
added y/n prompt #144
Conversation
Looks really good! I left some questions, I hope you don't mind! |
This needs some work anyway - MSVC is going crazy out of nowhere just because I included the |
@wolfv do you guys want the prompt to read on non-interactive terminals the input from the stdin? a use case would be: program | yes # <- send input via standard in It's already implemented (in theory) in CPP-Terminal, I'd just need to make it available to the prompt function. |
Hmm, good question. I think it woudl be nice, yeah, but most of the time in non-interactive mode we just use |
I have tested some fixes for the windows CI in #125 and removing the "platform.hpp" include in the "base.hpp" file fixes all errors (of course that is not a possible fix, as we need it to enable the "raw mode" for capturing input). I'll have to make some bigger changes for fixing this. Originally i planed on fixing that after having the prompt done, but well... |
3177a27
to
55fe9c4
Compare
…ggle to chose between both versions
55fe9c4
to
1212520
Compare
This PR is now ready @certik @wolfv. Please review it. I have fixed the windows error with a simple undefine for now. It looks ugly, but i'll have to finish the Platform header in order to fix that properly (which will be my next PR). I have moved adding support for reading the choice from stdin to #146. |
I have added CTRL+D support. Can you guys test this PR? |
So what's the status of this PR? |
Let me review this. |
It looks fine to me. @wolfv do you think this goes in the right direction? We can always improve upon it once it is merged. |
It has been two weeks now. @wolfv please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is fine.
Overall I don't like including windows header files in our header files, they should be included in cpp file only.
I suggest we merge this and iterate on it in master. And if improvements are needed, we can add them in later.
Yes, I notes that already. The problem with the windows header is that It's needed for saving the terminal state. I think that the right way would be to create a custom struct and then move the old terminal state from the struct defined in |
This PR adds some variations of a y/n style prompt.
Todo: