-
-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for ranges in the --accept
option / config field
#1167
feat: Add support for ranges in the --accept
option / config field
#1167
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a draft, but I thought I might add some early feedback. Keep it up! 😃
Co-authored-by: Matthias Endler <[email protected]>
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
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.
One thing that I would love to support would be a mix of both styles:
But I guess that's a trickier change. The reason is, that I'm a bit afraid of breaking changes, but then again I don't know many people who set multiple |
Ah that's neat! I usually include quotes just to be sure...
Yeah that's definitely a harder change. However, just like you mentioned, the chance people use multiple I can also open a separate PR, which explores further improvements, like merging multiple |
We didn't add 403 to the range of accepted status codes yet, or? |
No, not yet - but we can add it. How would we go about doing that? I think there are two ways:
|
Yeah, |
Okay, I will add this. Then we should be ready to merge! |
Added |
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 the default value into a string and then re-parses it via the `FromStr` trait. The `Display` impl from `AcceptSelector` included surrounding square brackets `[ ]` which failed to parse. cargo expand really helped here, as I had to dig into the code generated by the Clap `Parser` derive macro. I'm still unsure why Clap handles default values in this way, but there surely is a reason for it. I'm just too lazy to do further research into that.
Okay, this was quite a dig. Clap uses the default value not as is but will use 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 |
OMG, I would never have found that. Kudos! |
The config test failed because 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 I removed The final failing test will be a satisfying one, so I left it up to you. ❤️ |
Yeah, that's nice. I was kind of clueless about what exactly it complained about.
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 Let me know how we want to proceed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned out really great!
The flaky tests are a bit of an issue, but let's tackle that at a later point.
I tried to fix that once with multiple retries, but it seems that doesn't always do the trick.
We might have to disable these checks or put them into a different, non-default group. 🤷♀️
The changes in this PR make a lot of sense and the code quality is great, so I don't mind merging and releasing it. 🚀 Thanks for the contribution!
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 :( |
Adds support for accept ranges discussed in #1157. This allows the user to specify custom HTTP status codes accepted during checking and thus will report as valid (not broken). The accept option only supports specifying status codes as a comma-separated list. With this PR, the option will accept a list of status code ranges formatted like this:
These combinations will be supported:
..<end>
,..=<end>
,<start>..<end>
and<start>..=<end>
. The behaviour is copied from the RustRange
like concepts:..<end>
, includes 0 to<end>
(exclusive)..=<end>
, includes 0 to<end>
(inclusive)<start>..<end>
, includes<start>
to<end>
(exclusive)<start>..=<end>
, includes<start>
to<end>
(inclusive)This draft PR only implements some foundation work for this feature. Once all functionality is added, the PR will be ready to review.