-
Notifications
You must be signed in to change notification settings - Fork 413
feat: add ImpitHttpClient
http-client client using the impit
library
#1151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
For now, I suggest adding Awaiting a decision - apify/impit#123 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
pyproject.toml:63
- The version requirement for impit in the main dependencies (>=0.1.0) differs from the one in the adaptive-crawler section (>=0.2.0), which may lead to dependency conflicts. Consider aligning them to a consistent version.
"impit>=0.1.0",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Python binding The Replacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are following established pattern about adding new test file for the new client, but maybe now is the time to refactor the tests and have only one test file for all clients and parametrize all tests with client.
For example:
@pytest.mark.parametrize("http_client", [
CurlImpersonateHttpClient(http_version=CurlHttpVersion.V1_1),
ImpitHttpClient(),
HttpxHttpClient(http2=False)])
async def test_http_1(http_client: HttpClient, server_url: URL) -> None:
response = await http_client.send_request(str(server_url))
assert response.http_version == 'HTTP/1.1'
Maybe we would need 3xclient factories instead and parametrize by that, but regardless of that I think it would be great to reduce code duplication and ensure that we have exactly the same tests for all clients and that they can all work in exactly the same way.
But this is just a suggestion. Maybe it can be done in separate PR as well to not mix new implementation with pure refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea. But yes, I think it should be done in a separate PR.
Also, we could take the same approach for beautifulsoup
and parsel
crawlers. Since the tests for them are also completely duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will wait for this PR to be merged first, and then we can refactor: #1299
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Not merging this PR until we resolve the test issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test issue
[gw1] node down: Not properly terminated
[gw1] [ 98%] FAILED tests/unit/test_service_locator.py::test_storage_client_conflict
replacing crashed worker gw1
tests/unit/crawlers/_parsel/test_parsel_crawler.py::test_enqueue_links_selector[curl]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
ImpitHttpClient
http-client client using theimpit
libraryIssues
impit
as HTTP client #1079Testing
Added tests for
ImpitHttpClient
.ImpitHttpClient
is enabled for all tests using http-client