Skip to main content
added 343 characters in body
Source Link
  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer, which doesn't necessarily work. This appears to be a serious error; see this page for an explanation.

  • Also caught by clippy: functions don't need -> () (if there's no return type, then () is implicit).

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but I usually recommend using generics when possible because this avoids dynamic overhead at runtime. Instead you get a copy of your function for the actual concrete type that you need it, like with C++ templates. For consideration, here is what your Cat struct looks like using generics instead:

    struct Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        stdout: &'a mut F1,
        stderr: &'a mut F2,
    }
    
    impl<'a, F1, F2> Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
            Cat { stdout, stderr }
        }
    
    ...
    

    and in your TestFixture:

    fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
        return Cat::new(&mut self.stdout, &mut self.stderr);
    }
    

    You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • You are overusing .unwrap() -- fine for a toy project, but not really fine if you wanted to ship this as the real cat utility; it would occasionally fail and panic with no explanation. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions so as to make the failure model completely explicit.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

    for (line_number, line) in input.lines().enumerate() {
    

    If you do this though, you get numbering starting from zero. ToSo to get numbering starting from 1, so you have to do: let number_string = (line_number + 1).to_string()

  • I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:

    fn collect(self) -> (String, String) {
        let out = String::from_utf8(self.stdout.into_inner())
            .expect("invalid UTF8 found in stdout!");
        let err = String::from_utf8(self.stderr.into_inner())
            .expect("invalid UTF8 found in stderr!");
        (out, err)
    }
    

    Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing. Printing the help) would be more useful, and there is probably a way to configure structopt / your main function to have that behavior.

  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer, which doesn't necessarily work. This appears to be a serious error; see this page for an explanation.

  • Also caught by clippy: functions don't need -> () (if there's no return type, then () is implicit).

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but I usually recommend using generics when possible because this avoids dynamic overhead at runtime. Instead you get a copy of your function for the actual concrete type that you need it, like with C++ templates. For consideration, here is what your Cat struct looks like using generics instead:

    struct Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        stdout: &'a mut F1,
        stderr: &'a mut F2,
    }
    
    impl<'a, F1, F2> Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
            Cat { stdout, stderr }
        }
    
    ...
    

    and in your TestFixture:

    fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
        return Cat::new(&mut self.stdout, &mut self.stderr);
    }
    

    You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • You are overusing .unwrap() -- fine for a toy project, but not really fine if you wanted to ship this as the real cat utility; it would occasionally fail and panic with no explanation. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions so as to make the failure model completely explicit.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

    for (line_number, line) in input.lines().enumerate() {
    

    If you do this though, you get numbering starting from zero. To get numbering starting from 1, so you have to do let number_string = (line_number + 1).to_string()

  • I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:

    fn collect(self) -> (String, String) {
        let out = String::from_utf8(self.stdout.into_inner())
            .expect("invalid UTF8 found in stdout!");
        let err = String::from_utf8(self.stderr.into_inner())
            .expect("invalid UTF8 found in stderr!");
        (out, err)
    }
    

    Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing help).

  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer, which doesn't necessarily work. This appears to be a serious error; see this page for an explanation.

  • Also caught by clippy: functions don't need -> () (if there's no return type, then () is implicit).

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but I usually recommend using generics when possible because this avoids dynamic overhead at runtime. Instead you get a copy of your function for the actual concrete type that you need it, like with C++ templates. For consideration, here is what your Cat struct looks like using generics instead:

    struct Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        stdout: &'a mut F1,
        stderr: &'a mut F2,
    }
    
    impl<'a, F1, F2> Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
            Cat { stdout, stderr }
        }
    
    ...
    

    and in your TestFixture:

    fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
        return Cat::new(&mut self.stdout, &mut self.stderr);
    }
    

    You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • You are overusing .unwrap() -- fine for a toy project, but not really fine if you wanted to ship this as the real cat utility; it would occasionally fail and panic with no explanation. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions so as to make the failure model completely explicit.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

    for (line_number, line) in input.lines().enumerate() {
    

    If you do this though, you get numbering starting from zero. So to get numbering starting from 1: let number_string = (line_number + 1).to_string()

  • I like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:

    fn collect(self) -> (String, String) {
        let out = String::from_utf8(self.stdout.into_inner())
            .expect("invalid UTF8 found in stdout!");
        let err = String::from_utf8(self.stderr.into_inner())
            .expect("invalid UTF8 found in stderr!");
        (out, err)
    }
    

    Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing. Printing the help would be more useful, and there is probably a way to configure structopt / your main function to have that behavior.

added 343 characters in body
Source Link
  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer --, which doesn't necessarily work. This appears to be a serious error; see this page for an explanation. Also,

  • Also caught by clippy: functions don't need -> () (if there's no return type, then () is implicit).

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but personally I find it much cleaner to useusually recommend using generics when possible (andbecause this is also usually more efficient)avoids dynamic overhead at runtime. Instead you get a copy of your function for the actual concrete type that you need it, like with C++ templates. For consideration, here is what your Cat struct looks like using generics instead:

    struct Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        stdout: &'a mut F1,
        stderr: &'a mut F2,
    }
    
    impl<'a, F1, F2> Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
            Cat { stdout, stderr }
        }
    
    ...
    

    and in your TestFixture:

    fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
        return Cat::new(&mut self.stdout, &mut self.stderr);
    }
    

    You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

struct Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    stdout: &'a mut F1,
    stderr: &'a mut F2,
}

impl<'a, F1, F2> Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
        Cat { stdout, stderr }
    }

...

and in your TestFixture:

fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
    return Cat::new(&mut self.stdout, &mut self.stderr);
}

You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • I agree that youYou are probably overusing .unwrap() -- fine for a toy project, but not necessarilyreally fine if you wanted to ship this as the real cat utilityutility; it would occasionally fail and panic with no explanation. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions andso as to make explicit the failure model completely explicit.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

    for (line_number, line) in input.lines().enumerate() {
    

    If you do this though, you get numbering starting from zero. To get numbering starting from 1, so you have to do let number_string = (line_number + 1).to_string()

for (line_number, line) in input.lines().enumerate() {

If you do this though, you get numbering starting from zero. To get numbering starting from 1, so you have to do let number_string = (line_number + 1).to_string()

  • I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:

    I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:

    fn collect(self) -> (String, String) {
        let out = String::from_utf8(self.stdout.into_inner())
            .expect("invalid UTF8 found in stdout!");
        let err = String::from_utf8(self.stderr.into_inner())
            .expect("invalid UTF8 found in stderr!");
        (out, err)
    }
    

    Then use it like: assert_eq!(expected_error, fixture.collect().1).

fn collect(self) -> (String, String) {
    let out = String::from_utf8(self.stdout.into_inner())
        .expect("invalid UTF8 found in stdout!");
    let err = String::from_utf8(self.stderr.into_inner())
        .expect("invalid UTF8 found in stderr!");
    (out, err)
}

Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing help).

    Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing help).

  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer -- see this page for an explanation. Also, functions don't need -> () (if there's no return type, then () is implicit)

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but personally I find it much cleaner to use generics when possible (and this is also usually more efficient). For consideration, here is what your Cat struct looks like using generics instead:

struct Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    stdout: &'a mut F1,
    stderr: &'a mut F2,
}

impl<'a, F1, F2> Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
        Cat { stdout, stderr }
    }

...

and in your TestFixture:

fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
    return Cat::new(&mut self.stdout, &mut self.stderr);
}

You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • I agree that you are probably overusing .unwrap() -- fine for a toy project, but not necessarily fine if you wanted to ship this as the real cat utility. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions and make explicit the failure model.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

for (line_number, line) in input.lines().enumerate() {

If you do this though, you get numbering starting from zero. To get numbering starting from 1, so you have to do let number_string = (line_number + 1).to_string()

  • I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:
fn collect(self) -> (String, String) {
    let out = String::from_utf8(self.stdout.into_inner())
        .expect("invalid UTF8 found in stdout!");
    let err = String::from_utf8(self.stderr.into_inner())
        .expect("invalid UTF8 found in stderr!");
    (out, err)
}

Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing help).
  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer, which doesn't necessarily work. This appears to be a serious error; see this page for an explanation.

  • Also caught by clippy: functions don't need -> () (if there's no return type, then () is implicit).

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but I usually recommend using generics when possible because this avoids dynamic overhead at runtime. Instead you get a copy of your function for the actual concrete type that you need it, like with C++ templates. For consideration, here is what your Cat struct looks like using generics instead:

    struct Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        stdout: &'a mut F1,
        stderr: &'a mut F2,
    }
    
    impl<'a, F1, F2> Cat<'a, F1, F2>
    where
        F1: io::Write,
        F2: io::Write,
    {
        fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
            Cat { stdout, stderr }
        }
    
    ...
    

    and in your TestFixture:

    fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
        return Cat::new(&mut self.stdout, &mut self.stderr);
    }
    

    You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • You are overusing .unwrap() -- fine for a toy project, but not really fine if you wanted to ship this as the real cat utility; it would occasionally fail and panic with no explanation. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions so as to make the failure model completely explicit.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

    for (line_number, line) in input.lines().enumerate() {
    

    If you do this though, you get numbering starting from zero. To get numbering starting from 1, so you have to do let number_string = (line_number + 1).to_string()

  • I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:

    fn collect(self) -> (String, String) {
        let out = String::from_utf8(self.stdout.into_inner())
            .expect("invalid UTF8 found in stdout!");
        let err = String::from_utf8(self.stderr.into_inner())
            .expect("invalid UTF8 found in stderr!");
        (out, err)
    }
    

    Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing help).

Source Link

  • First things first -- run cargo clippy! Generally speaking its suggestions are quite helpful. In this case clippy raises some errors because you are using write instead of write_all to write an entire buffer -- see this page for an explanation. Also, functions don't need -> () (if there's no return type, then () is implicit)

  • The use of structopt is very clean and natural and fits the task, as well as implementing Default for your CLI arguments for convenient initialization.

  • A stylistic point: your code is littered with &'a mut dyn Trait objects everywhere. Some Rust programmers write code like this, so I wouldn't say it's necessarily wrong, but personally I find it much cleaner to use generics when possible (and this is also usually more efficient). For consideration, here is what your Cat struct looks like using generics instead:

struct Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    stdout: &'a mut F1,
    stderr: &'a mut F2,
}

impl<'a, F1, F2> Cat<'a, F1, F2>
where
    F1: io::Write,
    F2: io::Write,
{
    fn new(stdout: &'a mut F1, stderr: &'a mut F2) -> Self {
        Cat { stdout, stderr }
    }

...

and in your TestFixture:

fn make_cat(&mut self) -> Cat<impl io::Write, impl io::Write> {
    return Cat::new(&mut self.stdout, &mut self.stderr);
}

You can similarly replace your &mut dyn BufRead objects, e.g. fn cat<G: BufRead>(&mut self, config: &CatConfig, input: &mut G) {

  • I agree that you are probably overusing .unwrap() -- fine for a toy project, but not necessarily fine if you wanted to ship this as the real cat utility. Since this is a binary, the easiest fix is just to use .expect("Explain what bad happened here") instead of unwrap -- this should be propagated back to the user. For a library you would want to return Result<(), String> or similar for all your functions and make explicit the failure model.

  • In the fn cat function, you are manually incrementing a counter; consider the alternative

for (line_number, line) in input.lines().enumerate() {

If you do this though, you get numbering starting from zero. To get numbering starting from 1, so you have to do let number_string = (line_number + 1).to_string()

  • I actually like your use of TestFixture: you need to collect output during the lifetime of a test, so it makes perfect sense to make an object that represents that lifetime. One simplification to consider though is avoiding the cloning shenanigans in functions get_output and get_error: instead you could consume the object after it's done (after all, once you collect the output you don't need it anymore), like this:
fn collect(self) -> (String, String) {
    let out = String::from_utf8(self.stdout.into_inner())
        .expect("invalid UTF8 found in stdout!");
    let err = String::from_utf8(self.stderr.into_inner())
        .expect("invalid UTF8 found in stderr!");
    (out, err)
}

Then use it like: assert_eq!(expected_error, fixture.collect().1).

  • Finally, I found it counterintuitive that running the binary with no arguments prints nothing (not printing help).