fix: Handle invalid URLs in RequestList#1803
Conversation
Mantisus
changed the title
RequestListRequestList
|
|
||
| async def test_handle_invalid_url() -> None: | ||
| """Test that invalid URLs are handled gracefully.""" | ||
| request_list = RequestList(['invalid-url.com', 'https://valid.placeholder.com']) |
There was a problem hiding this comment.
Why not running the validation even earlier?
Is there any point why should this RequestList(['invalid-url.com']) be even allowed?
Maybe we could warn/raise already here?
There was a problem hiding this comment.
RequestList can pull the URLs from any iterable, so validation just before returning "next request" is non-negotiable. We can validate in the constructor as well though, I guess.
There was a problem hiding this comment.
This could be a very long list, and if we iterate fully in the constructor just for validation, it feels like an overhead. However, if we wrap it in a generator that performs the validation, that would essentially be the current solution. This is because, with persistence disabled, validation occurs the moment a new URL is received from the iterator. And when persistence is enabled, we iterate through all URLs before saving in the storage.
There was a problem hiding this comment.
I am fine with this explanation, thanks
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
LGTM, but what @Pijukatel says is legit
4 participants
Description
RequestList.Issues
RequestListwith persistence doesn't handle malformed URLs correctly #1802Testing