-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 std::io::input
simple input function.
#75435
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Decided to create a new pull request due to my incompetence and inability to fix merge conflicts with :( #74178 |
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.
Thanks for submitting this change again! I guess we should close the old PR then?
I left a few inline comments, but I am not quite happy with this function as is, unfortunately :/
(This has little to do with your particular changes, @sHaDoW-54, so please don't take the rest of the comment personally 🙂)
- The name
input
doesn't feel right to me. I would much preferread_line
as suggested here. - I prefer having two methods, one with prompt and one without; passing an empty string really doesn't feel like a good API IMO.
- I think we should also think about a function using
FromStr
to parse the read string into the type that the user expects. This is required in many "beginner situations" (from my experience), including the Guessing Game chapter in the Rust book. This can very well be an additional function, but I think it should factor into the API design.
As I argued in your previous PR, I would prefer seeing an RFC for this API. There has been a lot of "prior art" (both, in external crates and other languages), and the design space is quite large. I think a proper summary would aid discussion a lot. People in the last PR rather disagreed with that, saying that an RFC would be overkill.
The forge currently says:
New unstable features don't need an RFC before they can be merged. If the feature is small, and the design space is straightforward, stabilizing it usually only requires the feature to go through FCP. Sometimes however, you may ask for an RFC before stabilizing.
I don't think "the design space is straightforward", which would imply an FCP is not sufficient for stabilization. We can of course merge it already since it's unstable anyway, but I don't really see the point of that if the API might very well be completely changed.
I would appreciate your feedback (mostly on the meta issue) @rust-lang/libs
Co-authored-by: Lukas Kalbertodt <lukas.kalbertodt@gmail.com>
trailing whitespace
@LukasKalbertodt Is this what you meant with using It allows the user to define the type that the function returns. fn read_line<T: std::fmt::Display + std::fmt::Debug>(
prompt: &str,
) -> std::result::Result<T, <T as std::str::FromStr>::Err>
where
T: std::str::FromStr,
<T as std::str::FromStr>::Err: std::fmt::Debug,
{
let stdout = stdout();
let mut lock = stdout.lock();
lock.write_all(prompt.as_bytes())
.expect("failed to write whole buffer");
lock.flush().expect("failed to flush stdout");
let mut input = String::new();
stdin().read_line(&mut input).expect("failed to read stdin");
input.trim_end_matches(&['\n', '\r'][..]).parse::<T>()
} #[macro_export]
macro_rules! read_line {
() => {{
let mut input = String::new();
match stdin().read_line(&mut input).unwrap() {
0 => Err(Error::new(
ErrorKind::UnexpectedEof,
"input reached eof unexpectedly",
)),
_ => Ok(String::from(input.trim_end_matches(&['\n', '\r'][..]))),
}
}};
($arg1:expr) => {{
let stdout = stdout();
let mut lock = stdout.lock();
lock.write_all($arg1.as_bytes())
.expect("failed to write whole buffer");
lock.flush().expect("failed to flush stdout");
let mut input = String::new();
match stdin().read_line(&mut input).expect("failed to read stdin") {
0 => Err(Error::new(
ErrorKind::UnexpectedEof,
"input reached eof unexpectedly",
)),
_ => Ok(String::from(input.trim_end_matches(&['\n', '\r'][..]))),
}
}};
} |
As Pickfire explained here A macro such as this, might be best for simplifying the function and allowing the user to specify the type optionally. #[macro_export]
macro_rules! read_line {
() => {{
let mut input = String::new();
match stdin().read_line(&mut input).expect("failed to read stdin") {
0 => Err(Error::new(
ErrorKind::UnexpectedEof,
"input reached eof unexpectedly",
)),
_ => Ok(String::from(input.trim_end_matches(&['\n', '\r'][..]))),
}
}};
($type1:ty) => {{
let mut input = String::new();
stdin().read_line(&mut input).expect("failed to read stdin");
input.trim_end_matches(&['\n', '\r'][..]).parse::<$type1>()
}};
($arg1:expr) => {{
let stdout = stdout();
let mut lock = stdout.lock();
lock.write_all($arg1.as_bytes())
.expect("failed to write whole buffer");
lock.flush().expect("failed to flush stdout");
let mut input = String::new();
match stdin().read_line(&mut input).expect("failed to read stdin") {
0 => Err(Error::new(
ErrorKind::UnexpectedEof,
"input reached eof unexpectedly",
)),
_ => Ok(String::from(input.trim_end_matches(&['\n', '\r'][..]))),
}
}};
($type1:ty,$arg1:expr) => {{
let stdout = stdout();
let mut lock = stdout.lock();
lock.write_all($arg1.as_bytes())
.expect("failed to write whole buffer");
lock.flush().expect("failed to flush stdout");
let mut input = String::new();
stdin().read_line(&mut input).expect("failed to read stdin");
input.trim_end_matches(&['\n', '\r'][..]).parse::<$type1>()
}};
} Review and suggest any changes before i commit this one. |
It's close to what I had in mind. There are a couple of design possibilities here. Your code currently panics on IO errors, something we certainly don't want. We could:
I'm not a fan of the two macro based possibilities you posted. Macros in general could be a path forward, though. I am still kind of waiting for input on how to proceed with this PR. Maybe I will find the time to write a summary about all possible API designs I can think of, but I won't promise. |
I found some time and wrote a couple of things about "prior art", including other languages and Rust crates. This PR is probably not the best place to put it, but I figured it could help in this whole discussion. If there is a bug in my comment, or something can be improved, or I should add something: please tell me (or directly edit this comment if you have the GitHub privileges). Prior art(stdout flushing is ignored in all examples) In other languages(I am by no means an expert in all these languages. I just picked what I thought was "the main way" to read values from stdin. Mostly by googling "{lang} guessing game".) C:
|
Can prompt | Can parse | Can match literal strings | Can read multiple values | |
---|---|---|---|---|
C: scanf |
✗ | ✔ | ✔ | ✔ |
C++: iostreams | ✗ | ✔ | ✗ | ✔ |
Java: Scanner |
✗ | ✔ | (✔) | (✔) |
JavaScript (Node) | ✔ | ✗ | ✗? | (✔) |
Python: input |
✔ | ✗ | ✗ | ✗ |
Ruby | ✗ | ✗ | ✗ | ✗ |
--- | ||||
crate: text_io |
✗ | ✔ | ✔ | ✔ |
crate: scan_rules |
✗ | ✔ | ✔ | ✔ |
crate: scan_fmt |
✗ | ✔ | ✔ | ✔ |
@LukasKalbertodt Just wondering if it is reading as Also, is it worth adding |
add from str
add extra func
imports
unused import
format
☔ The latest upstream changes (presumably #77154) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
merge conflicts
@sHaDoW-54 Can you please move the latest changes in stdio to a second pull request? Also, I wonder why the tests was removed (can comment on the other pull request). At least we can keep this pull request only to |
library/std/src/io/stdio.rs
Outdated
match stdin().read_line(&mut input)? { | ||
0 => Err(Error::new(ErrorKind::UnexpectedEof, "input reached eof unexpectedly")), | ||
_ => { | ||
input.pop(); |
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.
Considering stdin().read_line()
does not drop newlines, shouldn't this function keep those in tact as well? Might be a bit confusing that stdin().read_line()
gives a line including the newline, and io::read_line()
gives one without the newline.
I agree it's probably more useful if it already strips the newline. But I'm worried about the inconsistency between two functions with the same name.
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 agree the inconsistency between stdin().read_line()
and io::read_line()
is concerning, but I think wanting to keep the newline would be in the minority and could be corrected with this snippet of code, perhaps we can avoid confusion by changing the name of the function?
if cfg!(windows) {
string.push_str("\r\n");
} else if cfg!(unix) {
string.push('\n');
}
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.
could be corrected with this snippet of code
No, that unconditionally adds a \r\n
or \n
. The original line might or might not end in a newline (e.g. at the end of the stream/file), and might be just a \n
on Windows in certain circumstances. This is why the already existing read_line
function keeps the newlines, to not throw away any information.
Anyway, I agree that a function that gives a line with the newline stripped is useful. But considering the existing read_line
keeps the newlines, this shouldn't have different behaviour while using the same name. (Also, the existing read_line
returns Ok
for zero bytes of input, while this function returns an Err
in that case.)
Is there an RFC for this? It's a bit hard right now to keep track of all the design decisions like the name, newline stripping, the possibility of parsing things, showing a prompt or not, etc. Combined with the previous PR, this has now had 108 commits/versions. Maybe it's better to first agree on the interface/api before implementing it.
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.
That seems like a good plan, i believe it was mentioned in the previous PR that there have been RFC's on this before.
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.
A better choice may be to change the naming to reflect that it does not include the new lines, at least that could solve the problem with naming inconsistency.
But if we keep the line, then people would need to fallback to trim
which currently does not return a String
in place but &str
.
newline remover fix
☔ The latest upstream changes (presumably #78227) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
Closing this, since (looking at the discussion above and in the last PR) the design of such an api is best discussed first in an RFC. |
@sHaDoW-54 Are you interested to take up the task to write an RFC? It could be quite time-consuming but that is the best way to discuss the design of the API. If not, I could probably try writing it, I wanted to try writing one. @m-ou-se Do we need to write a pre-RFC first or we can just write a RFC directly? |
None of these things have strict rules, but I can definitely recommend discussing an RFC for this on the internals forum first. The main point of an RFC for this feature is to discuss all the design decisions and alternatives and to come to a conclusion which design would work best. A discussion on the forum would be a good way to gather ideas from other users to make sure the RFC is complete. |
@pickfire I planned to write this RFC myself for many months already but never found the time to actually do it. If you want to, we could work on that together or at least talk about some ideas. But of course, you can also do everything yourself! In any case, I would appreciate a ping via mail or Zulip if/when you start working on it :) To be honest, at this point, I would probably write an RFC. There have been a couple of pre-RFCs and internal threads about this already AFAIK, so not sure how useful it would be to create another one. (Just my opinion tho) |
https://hackmd.io/@G8ZK5BSuQPOxvEQWVZSxng/By2UUacFD/edit I just share the link, in the meantime I will work on that, if anyone have any other ideas or want to improve it can just edit. Thanks to @sHaDoW-54 for keeping up from the old pull request to this new pull requests and tons of review comments. |
Thank you @pickfire, It is much better than anything I could write. |
@LukasKalbertodt @sHaDoW-54 I wrote 3 methods based on the proposals you all wrote (and modified a bit with code from other places) to take inputs, their pros and cons as well as different variations and designs. What do you all think? https://hackmd.io/@G8ZK5BSuQPOxvEQWVZSxng/By2UUacFD/edit (I got lazy and wrote pros and cons as After looking at https://github.com/conradkleinespel/rprompt/blob/master/src/lib.rs, I noticed that After writing the (incomplete - prior arts and maybe something else) proposal, I believe that it may be better to have it as an external crate, but one thing I believe is that the crate should be able to educate users on input, io and buffers which the book and the docs is not able to achieve. CC @conradkleinespel author of |
I opened the RFC 3196 to propose a simple You're of course all more than welcome to comment on my RFC :) |
Condenses the normal input process into one function. This is good for new rust users who don't understand the normal process and for users who don't require special functionality.
Especially with new rust users looking to make small programs, input can be a tricky thing for them to grasp, especially when other languages do this much simpler.
EX:
Python : user_input = input("Enter: ")
Ruby: user_input = gets
C#: user_input = Console.ReadLine();
So this...
Would turn into this...