The new accept syntax is now included in the CLI. I opted to use a string of comma-separated ranges instead of a Vec of ranges. This allows users to specify ranges like --accept "100..=199,200,404" instead of

--accept "100..=199" --accept 200 --accept 404

One of the tests keeps failing, which boggles my mind because the test case should be unaffected by my changes. Maybe you can take a look @mre

Works for me.
BTW, it also works without quotes, it seems:

lychee --accept 100..=199,200,404 -- README.md

One thing that I would love to support would be a mix of both styles:

lychee --accept 100..=199,200 --accept 404 -- README.md

But I guess that's a trickier change.
We'd have to accept a Vec of AcceptSelector in the options and merge them. 🤔

The reason is, that I'm a bit afraid of breaking changes, but then again I don't know many people who set multiple accept args right now, so it might be fine?

Works for me. BTW, it also works without quotes, it seems:

lychee --accept 100..=199,200,404 -- README.md

Ah that's neat! I usually include quotes just to be sure...

One thing that I would love to support would be a mix of both styles:

lychee --accept 100..=199,200 --accept 404 -- README.md

But I guess that's a trickier change. We'd have to accept a Vec of AcceptSelector in the options and merge them. 🤔

The reason is, that I'm a bit afraid of breaking changes, but then again I don't know many people who set multiple accept args right now, so it might be fine?

Yeah that's definitely a harder change. However, just like you mentioned, the chance people use multiple --accept parameters is slim. So yes, we should be good to go.

I can also open a separate PR, which explores further improvements, like merging multiple AcceptSelectors and improving the internal merging of AcceptRanges in AcceptSelector.

We didn't add 403 to the range of accepted status codes yet, or?
That was at least my intention when I opened #1157.
Should we add 403 in this pull request?

No, not yet - but we can add it. How would we go about doing that? I think there are two ways:

• Providing a Default impl for AcceptSelector and changing the accept argument to AcceptSelector instead of Option<AcceptSelector>. This also requires us to use the clap default value attribute.
• We could provide a set of pre-defined accepted status codes here

  lychee/lychee-bin/src/client.rs

  Line 38 in 4bdf33d

  None => None,

Yeah, Default is the idiomatic way to go imho.

Okay, I will add this. Then we should be ready to merge!

Added Default and Display for AcceptSelector. The CLI now accepts AcceptSelector and uses default_value_t when the user doesn't provide a custom value.

Some failing unit tests. Apart from that looks great.

Okay, this was quite a dig. Clap uses the default value not as is but will use to_string (from the Display impl) to convert it into a string and then re-parses it via the FromStr trait. See generated code here:

let arg = arg
    .help("A List of accepted status codes for valid links")
    .long_help(None)
    .short('a')
    .long("accept")
    .default_value({
        static DEFAULT_VALUE: clap::__derive_refs::once_cell::sync::Lazy<String> =
            clap::__derive_refs::once_cell::sync::Lazy::new(|| {
                let val: AcceptSelector =
                    <AcceptSelector as ::std::default::Default>::default();
                ::std::string::ToString::to_string(&val)
            });
        let s: &'static str = &*DEFAULT_VALUE;
        s
    })
    // ...

The Display impl from AcceptSelector included surrounding square brackets [ ], which failed to parse.

OMG, I would never have found that. Kudos!

The config test failed because fixtures/configs/cache.toml didn't have an accept entry.
I've extended the error message to make that easier to troubleshoot in the future:

Running `target/debug/lychee --config fixtures/configs/cache.toml .`
[ERROR] Error while loading config file "fixtures/configs/cache.toml": Failed to parse configuration file: TOML parse error at line 1, column 1
  |
1 | cache = true
  | ^^^^^^^^^^^^
missing field `accept`

The default doesn't get picked up, because there is no default value for serde deserialization.
Notably, this is different from clap's default for the command-line; it has to be set separately.

I removed accept from pub(crate) fn merge(&mut self, toml: Config) { ... } and added the serde default.
My guess is, that we could also remove some other stuff from merge, but I didn't touch that for this PR.

The final failing test will be a satisfying one, so I left it up to you. ❤️
It is about adding the new default to the config file. Check the failing test to see what I mean. 😉 🚀
Good job!

I've extended the error message to make that easier to troubleshoot in the future

Yeah, that's nice. I was kind of clueless about what exactly it complained about.

The final failing test will be a satisfying one, so I left it up to you. ❤️
It is about adding the new default to the config file. Check the failing test to see what I mean. 😉 🚀

Ha! Thanks for saving me the last error 😏

I will look into it!

All tests should now run without issues. However, the failing tests (3) succeeded perfectly on my local machine. They are all related to Wayback testing. The test cases even state that they are flaky.

Let me know how we want to proceed.

Thanks! I loved working on this. Can't wait to tackle future improvements :)

This PR creates a breaking change between lychee 0.13.0 and 0.14.0 :(

#1338