feat: show redirects & remaps#2094
Conversation
de5c60a to
b0d9566
Compare
katrinafyi
left a comment
There was a problem hiding this comment.
It looks reasonable and seems to do what it should. The biggest comment is about whether the remap history needs to/should be globally stored. I think it shouldn't.
In testing, I also noticed an interesting interaction with caching:
a.md:
[a](https://httpbin.org/delay/5)
[a](https://httpbin.org/delay/5)
$ cargo run -- a.md --max-concurrency=1 --remap 'httpbin nowherejiosad'
[a.md]:
[ERROR] https://httpbin.org/delay/5 (at 2:1) | Error (cached)
[ERROR] https://nowherejiosad.org/delay/5 (at 1:1) | Connection failed. Check network connectivity and firewall settings | Remapped: https://httpbin.org/delay/5 --> https://nowherejiosad.org/delay/5)The remap is only printed for the non-cached run, and the cache hits have the old URL and no remap history. This makes sense, given the caching strategy, but it is a bit odd. But this also isn't new in this PR - on main, it's identical but for the "Remapped:" text so no need to change here.
It is also possible to remap URLs into excluded URLs. With -v, the excluded URLs are printed but they don't have redirect information. I think the [EXCLUDED] message uses the same formatter as the CLI output, so it would be nice to have remaps mentioned here as well.
$ cargo run -- a.md --max-concurrency=1 --remap 'httpbin nowherejiosad' --exclude nowherejiosad -v
[EXCLUDED] https://nowherejiosad.org/delay/5 (at 1:1) | This is due to your 'exclude' and 'include' values
[EXCLUDED] https://nowherejiosad.org/delay/5 (at 2:1) | This is due to your 'exclude' and 'include' values
🔍 2 Total (in 2ms) ✅ 0 OK 🚫 0 Errors 👻 2 Excluded22d9df0 to
d9aba65
Compare
katrinafyi
left a comment
There was a problem hiding this comment.
Looks good! Thanks the fast changes! There's a smattering of comments, all just suggestions and very optional.
fe91a5a to
a273181
Compare
That's interesting. If the status is really loaded from the disk's cache I think it would be expected behaviour. But thinking about this specific case, I'm not sure this should happen at all. See #2103 I also realised that the |
a2b72ec to
93c20ad
Compare
mre
left a comment
There was a problem hiding this comment.
I'm okay with merging this once CI is green. It's a solid improvement. The open discussion points aren't super critical afaict and we have issues to track those. Good work! 👍
a21c14a to
d7d7bbe
Compare
The Redirected variant now has Box<Status> instead of a terminal StatusCode. In practice this means that redirection details are shown even when the status is considered erroneous. Fixes lycheeverse#2011
Remaps are now tracked via the `RemapHistory` helper struct. This allows us to embed the information in the newly added `Status::Remapped` variant. Error logging and formatting has also been improved quite a bit, removing redundant information and displaying more useful details in some cases.
Remove unnecessary RemapHistory. Thanks @katrinafyi for the idea. Use Uris instead of Urls. Fix a bug where the original Uris were dumped instead of the remappings.
Co-authored-by: katrinafyi <39479354+katrinafyi@users.noreply.github.com>
d7d7bbe to
a15e7b7
Compare
Fixes #1493
Fixes #2011
Please take a look a the commit messages for more details.
Example
Running
echo 'http://github.com/lycheeverse/lychee' | cargo run - --remap 'github gitlab'will now yield:Notes
)in the details "Remapped: A --> B)", will adjust and write a test to cover this. It's currently uncovered.