support error in context of response - #781
Conversation
|
What's the expected use case? Abort further processing and retry the request if you notice something's wrongin one of the |
StantonMatt
left a comment
There was a problem hiding this comment.
I took a local pass because this changes the public callback APIs and WGH- asked about the expected use case.
On the stale head f1303d0, these pass:
- go test . -run TestResponseContextError -count=1 -v
- go test ./... -count=1
- git diff --check origin/master...HEAD
The blocker I see is API compatibility. Changing ResponseCallback, HTMLCallback, XMLCallback, ErrorCallback, and ResponseHeadersCallback to return error means existing callback registrations no longer compile. For example, current source passes go test ./_examples/proxy_switcher, but this branch fails with _examples/proxy_switcher/proxy_switcher.go:23:15 because func(r *colly.Response) is no longer assignable to ResponseCallback. The same signature break shows up broadly across checked-in examples such as basic, error_handling, request_context, scraper_server, and shopify_sitemap.
The branch also has content conflicts with current master in colly.go, colly_test.go, and http_backend.go.
Given that every existing callback user would need to add a return value, I do not think this shape is mergeable as a v2 change unless maintainers intentionally want a breaking API. A smaller path may be a separate opt-in hook for handlers that want to return an error, while keeping the existing callback types source-compatible.
more scene, we don't want stop runing in a page request faild, so we can use onerror log or add retry queue and continue the spider.