echo from scratch #32
Conversation
|
My first time ever creating a pull request on an open source project. Not a 100% sure about the contributing and code parity guidelines. |
|
Hi @playerdefault, thanks for submitting this! Congrats on your first PR! I'd be happy to merge this as soon it code reviews and passes the test suite. |
coreutils/echo.rs
Outdated
| fn stpcpy(_: *mut libc::c_char, _: *const libc::c_char) -> *mut libc::c_char; | ||
| pub fn echo_main() { | ||
| let mut output = String::new(); | ||
| let args: Vec<String> = env::args().collect(); |
There was a problem hiding this comment.
Instead of doing this manual args parsing, we should probably use SafeStyle instead. Here's an example: https://github.com/samuela/rustybox/blob/master/applets/applet_tables.rs#L2782 and https://github.com/samuela/rustybox/blob/master/coreutils/true.rs.
There was a problem hiding this comment.
There are also a few different args parsing libraries in rust. Thus far rustybox hasn't really settled on one, but we should probably pick one here and stick with it.
There was a problem hiding this comment.
@samuela Regarding the question of which argument parser to go with, clap's author has given some suggestions in this Reddit comment: https://www.reddit.com/r/rust/comments/8i5k3l/looking_for_a_tiniest_args_parser/dyp8l13?utm_source=share&utm_medium=web2x. Clap also looks like a good, reliable, and wildly popular choice to go with.
There was a problem hiding this comment.
FYI getopts only works for UTF-8 strings, so you can’t process non-UTF-8 filenames with it. It looks like SafeStyle only accepts &str arguments though anyway.
There was a problem hiding this comment.
@samuela Just saw the updates on the thread. I am stuck somewhere, plus, I have some questions about the build and test parts of the project that would not be suitable to ask here. Do you have a way I could contact separately like a Slack channel, Microsoft Teams channel, email thread or something else?
There was a problem hiding this comment.
Sure, you can contact me over email at the email listed in my github account, and I'd be happy to answer any questions
There was a problem hiding this comment.
Hey @samuela sent you an email! Could you find some time to look at it? Since it's not from Gmail, it might be in your spam.
There was a problem hiding this comment.
rust-lang/getopts#97 (comment)
argh is quite small, and there's some traction there to support non-utf8. edit: sorry, meant to link to google/argh#33
There's also pico-args, which to my knowledge is the smallest out there (see comparisons). And it also supports non-utf8. Main downside being the help text isn't autogenerated.
Related discussion here.
There was a problem hiding this comment.
Thanks for the pointers @joshuarli ! I wasn't aware of the argh project. That may be a suitable alternative.
|
Looks like this is moving along nicely! Feel free to ping me once CI passes and you're ready for a code review! |
|
Stumbed upon my PR from 3 years ago. Never thought I'll be here where I am today when I originally started working on this. Closing this PR for completeness and adding my code to the GitHub void forever. |
Recreated echo from scratch in Rust. Not feature complete but current PR is for a working MVP. Eventual goal is feature parity with busybox's echo.