-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
Allow excluding cache based on status code #1403
base: master
Are you sure you want to change the base?
Conversation
Perfect. Great work! Perhaps the only thing I'd add is an integration test, similar to this: lychee/lychee-bin/tests/cli.rs Lines 794 to 858 in eff77d6
Probably it's just a matter of copy-pasting that block and changing the parameter, to test the new flag. |
I'm happy to do it, but it'll have to wait for a few days. I can then do it either in this PR, or in a new one. |
Just tested the changes locally. ❯❯❯ cargo run -- --verbose --cache https://lychee.cli.rs/
Finished dev [unoptimized + debuginfo] target(s) in 0.19s
Running `target/debug/lychee --verbose --cache 'https://lychee.cli.rs/'`
[INFO ] Cache is recent (age: 1m 6s, max age: 1d 0h 0m 0s). Using.
✔ [200] https://lychee.cli.rs/introduction/
✔ [200] https://lychee.cli.rs/#_top
✔ [200] https://lychee.cli.rs/favicon.svg
✔ [200] https://lychee.cli.rs/
✔ [200] https://lychee.cli.rs/_astro/logo.BJx6koUn.svg
✔ [200] https://lychee.cli.rs/sitemap-index.xml
✔ [200] https://lychee.cli.rs/_astro/index.fVW1leCO.css
✔ [200] https://lychee.cli.rs/
✔ [200] https://lychee.cli.rs/
✔ [200] https://lychee.cli.rs/_astro/logo.BJx6koUn_1qY8Fi.svg
✔ [200] https://github.com/lycheeverse/lycheeverse.github.io/edit/master/src/content/docs/index.mdx
✔ [200] https://github.com/lycheeverse/lychee/
🔍 12 Total (in 1s) ✅ 12 OK 🚫 0 Errors
~/C/p/l/lychee ❯❯❯ echo $?
0
~/C/p/l/lychee ❯❯❯ cat .lycheecache
~/C/p/l/lychee ❯❯❯ |
I've added the integration test we discussed, and that one seems to work. Something isn't adding up here. 🤔 |
Could that be related to a cache file already existing? (in which case, the issue could be that in the case of running tests with an existing cache, the file isn't written again?) |
31bc11e
to
8631028
Compare
I'm not sure whether there's still something missing in this PR? |
Sorry for the late response. The feature doesn't work right now.
Nope, tried in a directory without a cache file. I didn't have time to look at it, but I guess it has something to do with the defaults of If you have some time to investigate or test on your side, I would greatly appreciate the effort. :) |
Closes #1400
This introduces an option
--cache-exclude-status
, which allows specifying a range of HTTP status codes which will be ignored from the cache.