Conversation
vdusek
left a comment
There was a problem hiding this comment.
I agree in principle, but I'm not entirely sure we can do it. We should definitely test this change on the platform - run integration and E2E tests in the SDK.
I completely agree with this. I don't see a good reason why it shouldn't work, but better safe than sorry. Once this is ready, no need to wait for my review unless you decide to change the PR substantially. |
|
I can not start the E2E tests directly on the branch from forked repo. I created a copy of the branch and started the tests here: https://github.com/apify/crawlee-python/actions/runs/20028369926 |
|
tests in the SDK - https://github.com/apify/apify-sdk-python/actions/runs/20029622226/job/57435192024?pr=702 Edit: they're failing |
I'm not sure that protecting these attributes is as critical as, for example,
I'll check why it's falling. Edit: Now the tests are passed without falling |
makes sense to change it in user handler as well - e.g., you detect a weird error and want to tell crawlee not to retry the request anymore
yeah, I guess that it could make sense to change it to "redirect" the request to a different branch of the router 🤷
This actually doesn't make much sense - it's there for checking if the URL still matches the
You may want to change this when re-enqueueing a request.
This is debatable, maybe it's enough to allow setting it when creating the request? |
This setter is required for fix #1606. Since |
There was a problem hiding this comment.
I tested it again (apify/apify-sdk-python#702) with the current state, and it passes.
LGTM and thanks for thinking through all the options.

Description
Requestfields such asunique_key,method, and others will not be changed.