[oech3]

fn parse_u8_or_err is mixing logic for parsing u8 and processing the error message.
We don't need to test logic of .parse::<u8> if they are separated.

[xtqqczze]

What is purpose of this refactoring; is it intended to support further changes?

[oech3]

No. This PR just drops tests we don't need to verify.

[xtqqczze]

In that case would you drop the refactoring and just remove the test.

[oech3]

No. If fn parse_u8_or_err contains parsing, we need to test parsing logic again.

[xtqqczze]

No. If fn parse_u8_or_err contains parsing, we need to test parsing logic again.

In that case, I don't think this change is justified. The refactoring makes the code more difficult to understand without providing any meaningful benefit.

[cakebaker]

An alternative idea is to modify the proposed helper function a bit to also create the error, something like:

fn create_err(e: &std::num::ParseIntError, arg: &str) -> Box<dyn UError> {
    let msg = if *e.kind() == IntErrorKind::PosOverflow {
        translate!("stty-error-invalid-integer-argument-value-too-large", "value" => format!("'{arg}'"))
    } else {
        translate!("stty-error-invalid-integer-argument", "value" => format!("'{arg}'"))
    };
    USimpleError::new(1, msg)
}

And then use it like:

let n = line.parse::<u8>().map_err(|e| create_err(&e, line))?;

[oech3]

Including USimpleError is fine. But we still repeat line variable.

[xtqqczze]

I don't think this refactoring solely to remove a test is worthwhile. Could we keep the parsing logic returning a Result instead, something like:

fn try_parse_u8(arg: &str) -> Result<u8, T>

[oech3]

We don't have similar fn for u32, usize, etc... yet. But I don't like to have a fn specialized for parsing u8.

[cakebaker]

I think the reduction of tests is "only" a side effect of the refactoring. For me it is about readability. The first thought when I see a call of parse_u8_or_err is: why not use parse::<u8>() from the std?

[oech3]

An another thought about this: we accepts different stderr with GNU. Then someone can try to patch GnuTests to accept raw parse::<u8>() at different PR based on this.

[xtqqczze]

The first thought when I see a call of parse_u8_or_err is: why not use parse::<u8>() from the std?

Right, the existing function is badly named, it could be something like: try_parse_integer_argument.

[sylvestre]

tests/by-util/test_stty.rs already checks both messages for line/min/time, do we still need these unit tests?

[oech3]

I'm fine to drop. But some tests are platform specific at there.