-
Couldn't load subscription status.
- Fork 18
RequestURL and ResponseURL #42
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
Changes from all commits
b2c665e
10a3883
f56da89
890a238
5b0e889
2884d2f
ea11f3d
ba4e615
aad8ae6
9287744
237fab6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,9 @@ | |
| import aiohttp.web_response | ||
| import pytest | ||
| import requests | ||
|
|
||
| import parsel | ||
|
|
||
| from web_poet import RequestUrl, ResponseUrl | ||
| from web_poet.page_inputs import ( | ||
| HttpRequest, | ||
| HttpResponse, | ||
|
|
@@ -72,7 +73,7 @@ def test_http_defaults(cls, body_cls): | |
| http_body = body_cls(b"content") | ||
|
|
||
| obj = cls("url", body=http_body) | ||
| assert obj.url == "url" | ||
| assert str(obj.url) == "url" | ||
| assert obj.body == b"content" | ||
| assert not obj.headers | ||
| assert obj.headers.get("user-agent") is None | ||
|
|
@@ -164,7 +165,8 @@ def test_http_headers_init_dict(cls, headers_cls): | |
|
|
||
| def test_http_request_init_minimal(): | ||
| req = HttpRequest("url") | ||
| assert req.url == "url" | ||
| assert str(req.url) == "url" | ||
| assert isinstance(req.url, RequestUrl) | ||
| assert req.method == "GET" | ||
| assert isinstance(req.method, str) | ||
| assert not req.headers | ||
|
|
@@ -189,12 +191,20 @@ def test_http_request_init_full(): | |
| http_body = HttpRequestBody(b"body") | ||
| req_2 = HttpRequest("url", method="POST", headers=http_headers, body=http_body) | ||
|
|
||
| assert req_1.url == req_2.url | ||
| assert str(req_1.url) == str(req_2.url) | ||
| assert req_1.method == req_2.method | ||
| assert req_1.headers == req_2.headers | ||
| assert req_1.body == req_2.body | ||
|
|
||
|
|
||
| def test_http_request_init_with_response_url(): | ||
| resp = HttpResponse("url", b"") | ||
| assert isinstance(resp.url, ResponseUrl) | ||
| req = HttpRequest(resp.url) | ||
| assert isinstance(req.url, RequestUrl) | ||
| assert str(req.url) == str(resp.url) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Gallaecio has raised a good point if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a test which shows that they're not equal. But I'm not sure this behavior is useful. What kind of errors would it prevent? I wonder if it'd better to leave it "undefined", instead of solidifying this behavior in tests (if that's even a reasonable approach :) pathlib does some magic when comparing paths, it's not a simple string match. E.g. on Windows path components are lower-cased before comparison. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test simply solidifies the expectation to users in case they're unsure. As a user, I might expect when initially using it for the first time that the expression I also expect such test to conveniently break when we override |
||
|
|
||
| def test_http_response_headers_from_bytes_dict(): | ||
| raw_headers = { | ||
| b"Content-Length": [b"316"], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ def async_mock(): | |
| """Workaround since python 3.7 doesn't ship with asyncmock.""" | ||
|
|
||
| async def async_test(req): | ||
| return HttpResponse(req.url, body=b"") | ||
| return HttpResponse(str(req.url), body=b"") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should be casting this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must admit I failed to make tests working, and applied this hack instead :) Mocks are not my friends. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking it's harmless, because tests should be getting ResponseUrl, as HttpResponse should be converting str to ResponseUrl; without having mocks in mind the change doesn't really change the behavior. But I'm likely missing some interactions with mocks which are in effect here. |
||
|
|
||
| mock.MagicMock.__await__ = lambda x: async_test(x).__await__() | ||
|
|
||
|
|
@@ -37,7 +37,7 @@ async def test_perform_request_from_httpclient(async_mock): | |
| response = await client.get(url) | ||
|
|
||
| # The async downloader implementation should return the HttpResponse | ||
| assert response.url == url | ||
| assert str(response.url) == str(url) | ||
| assert isinstance(response, HttpResponse) | ||
|
|
||
|
|
||
|
|
@@ -47,15 +47,15 @@ async def test_http_client_single_requests(async_mock): | |
|
|
||
| with mock.patch("web_poet.page_inputs.client.HttpRequest") as mock_request: | ||
| response = await client.request("url") | ||
| response.url == "url" | ||
| str(response.url) == "url" | ||
|
|
||
| response = await client.get("url-get", headers={"X-Headers": "123"}) | ||
| response.url == "url-get" | ||
| str(response.url) == "url-get" | ||
|
|
||
| response = await client.post( | ||
| "url-post", headers={"X-Headers": "123"}, body=b"body value" | ||
| ) | ||
| response.url == "url-post" | ||
| str(response.url) == "url-post" | ||
|
|
||
| assert mock_request.call_args_list == [ | ||
| mock.call( | ||
|
|
@@ -162,7 +162,7 @@ async def test_http_client_execute(async_mock): | |
| response = await client.execute(request) | ||
|
|
||
| assert isinstance(response, HttpResponse) | ||
| assert response.url == "url-1" | ||
| assert str(response.url) == "url-1" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import pytest | ||
|
|
||
| from web_poet._base import _Url | ||
| from web_poet import RequestUrl, ResponseUrl | ||
|
|
||
|
|
||
| def test_url_base_class(): | ||
| url_str = "http://example.com" | ||
| url = _Url(url_str) | ||
| assert str(url) == url_str | ||
| assert repr(url) == "_Url('http://example.com')" | ||
|
|
||
|
|
||
| def test_url_init_validation(): | ||
| with pytest.raises(TypeError): | ||
| _Url(123) | ||
|
|
||
|
|
||
| def test_url_subclasses(): | ||
| url_str = "http://example.com" | ||
|
|
||
| class MyUrl(_Url): | ||
| pass | ||
|
|
||
| class MyUrl2(_Url): | ||
| pass | ||
|
|
||
| url = MyUrl(url_str) | ||
| assert str(url) == url_str | ||
| assert url._url == url_str | ||
| assert repr(url) == "MyUrl('http://example.com')" | ||
|
|
||
| url2 = MyUrl2(url) | ||
| assert str(url2) == str(url) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('url_cls', [_Url, RequestUrl, ResponseUrl]) | ||
| def test_str_equality(url_cls): | ||
| url_str = "http://example.com#foo" | ||
| url = url_cls(url_str) | ||
| assert url != url_str | ||
| assert str(url) == url_str | ||
|
|
||
|
|
||
| def test_url_classes_eq(): | ||
| url_str = "http://example.com#foo" | ||
| request_url = RequestUrl(url_str) | ||
| response_url = ResponseUrl(url_str) | ||
|
|
||
| assert request_url != response_url | ||
| assert str(request_url) == str(response_url) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| """ | ||
|
|
||
|
|
||
| from typing import Type, TypeVar, List, Dict | ||
| from typing import Type, TypeVar, List, Dict, Union | ||
|
|
||
| from multidict import CIMultiDict | ||
|
|
||
|
|
@@ -32,3 +32,19 @@ def from_name_value_pairs(cls: Type[T_headers], arg: List[Dict]) -> T_headers: | |
| <_HttpHeaders('Content-Encoding': 'gzip', 'content-length': '648')> | ||
| """ | ||
| return cls([(pair["name"], pair["value"]) for pair in arg]) | ||
|
|
||
|
|
||
| class _Url: | ||
| """ Base URL class. | ||
| """ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hey @BurnzZ @Gallaecio! I tried to follow @Gallaecio's suggestion, and created a minimal version of the Url base class. To keep it minimal, I haven't added anything useful to the On one hand, it allows us to improve _Url class in future almost without any restrictions. On the other hand, it kind-of gets in a way now. With this implementation, users need to cast to str almost always: for example, HttpClient backends would always need to do So, for the users the current implementation is more cumbersome than just using |
||
| def __init__(self, url: Union[str, '_Url']): | ||
| if not isinstance(url, (str, _Url)): | ||
| raise TypeError(f"`url` must be a str or an instance of _Url, " | ||
| f"got {url.__class__} instance instead") | ||
| self._url = str(url) | ||
|
|
||
| def __str__(self) -> str: | ||
| return self._url | ||
|
|
||
| def __repr__(self) -> str: | ||
| return f"{self.__class__.__name__}({self._url!r})" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,8 +50,8 @@ class ResponseShortcutsMixin(SelectableMixin): | |
|
|
||
| @property | ||
| def url(self): | ||
| """Shortcut to HTML Response's URL.""" | ||
| return self.response.url | ||
| """Shortcut to HTML Response's URL, as a string.""" | ||
| return str(self.response.url) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for backwards compatibility, and also for consistency with .base_url and .urljoin properties, where urls are strings. Overall, I don't like ResponseShortcutsMixin, it seems using HttpResponse methods is better :) |
||
|
|
||
| @property | ||
| def html(self): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.