User hints#2021
Conversation
0fb03e7 to
977851b
Compare
977851b to
2e3d7f1
Compare
2e3d7f1 to
e90d978
Compare
e90d978 to
08f715f
Compare
|
@mre Thank you for your ideas. Initially I was thinking more of a fully functional approach were we wouldn't need to store state. But your idea also works. Currently user hints is limited to lychee-bin which makes sense and keeps things simple. But this might also be limiting as there is no way to "trigger" a user hint in lychee-lib. For example with reporting bot protection as mentioned in #2117 this might need to be updated. Then I was also considering a possibility to hide these hints. Some users might not want to see them or maybe there are scenarios where we shouldn't display them? I was considering:
Edit: ah maybe there's no need to any of this if instead we just use the logging level, so that they can be hidden with |
mre
left a comment
There was a problem hiding this comment.
I quite like it! Good work. The only remaining comment is optional.
Yeah, that would be great. In general, I'd wait until someone asks for it; but that might just be a "me" thing, haha. The functional approach also makes sense. While reading your comment, I thought about having a global registry of sorts, similar to how logging gets set up once at the beginning of a binary and then is configured for the rest of the runtime. Maybe you thought of something similar. I haven't looked into it, but I'm guessing the magic would be in a global constant and a function call which reads/writes from/into that. TBH, I think the current implementation works well and what I like about it is that it's definitely an improvement and still simple enough to understand without obstructing the business logic. The fact that it works on stats is a really nice touch and that's what makes it so self-contained right now. So I'd have no objections if we merged this. |
|
I've made some small adjustments, feel free to take another look.
I like this approach. But I was already imagining the user asking for a way to hide the hints :)
Agree in the current state it would maybe be even easier. But as soon as we need to have additional ways to add hints (e.g. #2117) the current state is probably necessary. |
mre
left a comment
There was a problem hiding this comment.
Looks really good. Ready to merge from my side.
The exclusive '..' range syntax follows Rust semantics and excludes the upper bound, which is a common source of confusion (e.g. '301..302' only accepts 301, not 302). Emit a warning to help users notice the mismatch.
|
@mre Last updates:
|
| .and_then(|b| b.uri.domain()); | ||
|
|
||
| if default_host_config && let Some(domain) = first_rate_limited_domain { | ||
| hint!( |
There was a problem hiding this comment.
The little exclamation marks make this looks so pretty. 😊
Sorry if this isn't the right place to discuss this, but I always found the message
(this depends on your "accept" configuration)on every line a bit noisy. Maybe we should put it at the end, as we do with the GitHub token hint.[stdin]: [301] https://mock.httpstatus.io/301 | Rejected status code: 301 Moved Permanently: Redirects may have been limited by "max-redirects". [404] https://mock.httpstatus.io/404 | Rejected status code: 404 Not Found Hint: You can configure accepted response codes with the "accept" option".We could introduce a
Hintstruct that collects all of those hints (GitHub token, accept codes, redirects detected).Originally posted by @mre in #2012 (comment)
Closes #2178