Du size_format flag override

[cerdelen]

Building on top of the work in PR #10664 this pr will extend the override behaviour for all size_format flags/options.

Also see: #10664 (comment)

[ChrisDryden]

The test cases are great and testing around locally it seems great to me, I think we can make it a bit cleaner though. Do you think we could seperate into a helper function the parsing of the explicit block size argument?

fn parse_block_size_option(matches: &ArgMatches) -> UResult<Option<SizeFormat>> {
    matches
        .get_one::<String>(options::BLOCK_SIZE)
        .map(|s| {
            let bs = read_block_size(Some(s.as_ref()))?;
            if bs == 0 {
                return Err(std::io::Error::other(translate!(
                    "du-error-invalid-block-size-argument",
                    "option" => options::BLOCK_SIZE,
                    "value" => s.quote()
                ))
                .into());
            }
            Ok(SizeFormat::BlockSize(bs))
        })
        .transpose()
}

Then it can make that section below smaller

[cerdelen]

I found yet another bug ... gnu always errors if the value for block-size is erroneous while upstream main (1 commit before #10664) does not because it only looks at block-size if no other option is present.

I will incorporate a more concise version and look at your suggestions

[cerdelen]

The test cases are great and testing around locally it seems great to me, I think we can make it a bit cleaner though. Do you think we could seperate into a helper function the parsing of the explicit block size argument?

fn parse_block_size_option(matches: &ArgMatches) -> UResult<Option<SizeFormat>> {
    matches
        .get_one::<String>(options::BLOCK_SIZE)
        .map(|s| {
            let bs = read_block_size(Some(s.as_ref()))?;
            if bs == 0 {
                return Err(std::io::Error::other(translate!(
                    "du-error-invalid-block-size-argument",
                    "option" => options::BLOCK_SIZE,
                    "value" => s.quote()
                ))
                .into());
            }
            Ok(SizeFormat::BlockSize(bs))
        })
        .transpose()
}

Then it can make that section below smaller

We cannot use map on the get_one because in read_block_size() a fallback value is define in case the parameter is None. If we use this in .map() we would never get that fallback value.

[ChrisDryden]

How about you call the function to read the block size here and then you don't need to have the call into it later?

[cerdelen]

I called it even before to only use it once and then use the return value for unwrap_or().

Otherwise i didn't see a way to only call it once with the candidates as i feel like there was no sensible way to ensure it being the fallback in case no arg was present and we needed the fallback value from that call.

Obv we could rewrite the read_block_size func to not be the fallback either... maybe this would be a cleaner approach.

[ChrisDryden]

I think you can also combine these two sections by using the value_source since it will work for both string and flag

        matches
            .value_source(arg)
            .is_some_and(|s| s == clap::parser::ValueSource::CommandLine)
            .then(|| matches.indices_of(arg).and_then(|mut i| i.next_back()))
            .flatten()

[ChrisDryden]

So I'm giving a bunch of suggestions, not to say that there's a right way to do that, feel free to not use any of them I just think its getting a bit verbose and hard to read so I'm just going to throw a bunch of ideas and feel free to try any that you think would make it easier to read?

[cerdelen]

I wasn't experienced enough with the clap api to know about value_source(). This is ofc a massive improvement in code reusability and conciseness. Thanks!

[ChrisDryden]

Sweet that looks much cleaner, I think theres just some linting issues but LGTM