search

lycheeverse / lychee

⚡ Fast, async, stream-based link checker written in Rust. Finds broken URLs and mail addresses inside Markdown, HTML, reStructuredText, websites and more!
https://lychee.cli.rs
Apache License 2.0
2.15k stars 130 forks source link

Lychee expects string for accepted status codes #644

Closed vipulgupta2048 closed 2 years ago

vipulgupta2048 commented 2 years ago

With #636, Lychee add support for parsing status code using v0.10.0 I am getting the error 

➜  docs git:(vipul/update-lychee) ✗ lychee --version
lychee 0.10.0
➜  docs git:(vipul/update-lychee) ✗ lychee --config ./lychee.toml "README.md"
Error: invalid type: sequence, expected a string for key `accept` at line 1 column 1

When I am specifying status codes like:

# Comma-separated list of accepted status codes for valid links.
accept = [200, 301, 429]

The error goes away when I specify status codes as string which was the old format. I am trying to read the PR and make a patch for this. I feel like the parse_statuscodes function still expects string but not completely sure how the tests would have been passing then.

mre commented 2 years ago

Latest lychee with these changes wasn't released yet. 😉 It will be part of 0.11.0 since it's a breaking change. For now you need to use a string, yes. I can't remember if the current master still supports a string as an argument, though. If not, we could add that to make it backwards compatible.

vipulgupta2048 commented 2 years ago

Thanks, I should have checked the releases first to make sure if v0.10.0 includes this change or not. My bad. Closing.

mre commented 2 years ago

No worries. 😆

vipulgupta2048 commented 2 years ago

@mre Although I want to ask what is the expected outcome of having an accepted status code. Referring to #634, I have added 429 to accepted status codes, but the report and result still list "Too many network request" errors. Is that expected? Check out my config here https://github.com/balena-io/docs/pull/2317

mre commented 2 years ago

Hum... it should work like you specified it and it should accept 429 as a valid status code. Not sure what's up with that. Can you try to pass --accept 200,204,301,429 in the args? Maybe the config option doesn't get properly merged in.

vipulgupta2048 commented 2 years ago

That was much better, 429 is being clearly accepted when I ran using the command line arg. Logs show it being green and no 429 in the report @mre

mre commented 2 years ago

Okay then our merging is borked. I think we wanted to switch to https://github.com/mehcode/config-rs for config merging, but I can't remember the crate anymore and I can't find the issue. PRs to fix this are very welcome. I'll keep this open for now.

mre commented 2 years ago

@vipulgupta2048 can you test the latest master? I tested it locally and it merging works as expected for me. (In my test I overwrote the accepted status code by using a custom config file and it worked.)

mre commented 2 years ago

My config:

# Comma-separated list of accepted status codes for valid links.
accept = [200, 429]
mre commented 2 years ago

For the record, we could move to config-rs once clap is supported (https://github.com/mehcode/config-rs/issues/328)

vipulgupta2048 commented 2 years ago

Thanks @mre, I was out on vacation so couldn't act on this. I tried building master v0.10.1, tested and it works as expected. Made a PR to get this change merged. Closing the issue.

vipulgupta2048 commented 2 years ago

Hey @mre can you release a new patch of the lychee action? Having v0.10.1 inside?

tooomm commented 2 years ago

Hey @mre can you release a new patch of the lychee action? Having v0.10.1 inside?

@vipulgupta2048 lychee v0.10.1 is already the default since lychee-action v1.5.1 (released a few weeks ago). It looks like you're using that already in your PR?