Skip to content
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

reorganize page_inputs.py as a submodule; move HttpClient to it #37

Merged
merged 2 commits into from
May 6, 2022

Conversation

BurnzZ
Copy link
Contributor

@BurnzZ BurnzZ commented Apr 25, 2022

Stemming off from #22 (comment).

@BurnzZ BurnzZ requested a review from kmike April 25, 2022 12:26
_Body = Union[bytes, HttpRequestBody]


async def _perform_request(request: HttpRequest) -> HttpResponse:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be inside this module, as moving it outside would cause a "circular import problem".

It's because _perform_request() needs HttpRequest and HttpResponse from page_inputs for its annotations while HttpClient (which is now inside the page_inputs subpackage) needs _perform_request().

Copy link
Member

@kmike kmike Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that can be possible to work around:

  1. add from __future__ import annotations to the module which contains _perform_request
  2. add if typing.TYPE_CHECKING: from web_poet... import HttpRequest, HttpResponse

See https://mypy.readthedocs.io/en/stable/runtime_troubles.html#import-cycles and https://peps.python.org/pep-0563/#runtime-annotation-resolution-and-type-checking

On a first sight, having this function in client.py looks fine though :) Do you see a better location for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kmike , thanks for the suggestion! I was about to use your workaround but then simply importing HttpRequest and HttpResponse in requests.py is now working without the "circular import problem". I think the problem before was a bad ./tox cache in my local machine. 😅

On a first sight, having this function in client.py looks fine though :) Do you see a better location for this function?

It would seem that the _perform_request() function would be housed perfectly in web_poet/requests.py. This makes web_poet/page_inputs/client.py cleaner, containing code which emphasizes only on what users could use as page inputs.

Updated this in a4f1dcc.

@BurnzZ BurnzZ mentioned this pull request Apr 25, 2022
4 tasks
Base automatically changed from additional-requests to master April 28, 2022 04:03
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #37 (a4f1dcc) into master (753e6ad) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #37   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        14    +3     
  Lines          285       289    +4     
=========================================
+ Hits           285       289    +4     
Impacted Files Coverage Δ
web_poet/__init__.py 100.00% <100.00%> (ø)
web_poet/page_inputs/__init__.py 100.00% <100.00%> (ø)
web_poet/page_inputs/client.py 100.00% <100.00%> (ø)
web_poet/page_inputs/http.py 100.00% <100.00%> (ø)
web_poet/page_inputs/meta.py 100.00% <100.00%> (ø)
web_poet/requests.py 100.00% <100.00%> (ø)

@BurnzZ BurnzZ marked this pull request as ready for review April 28, 2022 11:10
@kmike kmike merged commit f2dce80 into master May 6, 2022
@kmike
Copy link
Member

kmike commented May 6, 2022

Thanks @BurnzZ!

@BurnzZ BurnzZ deleted the organize-page-inputs branch May 7, 2022 02:41
BurnzZ added a commit that referenced this pull request May 7, 2022
This adjusts the code after the said PR refactored the project
structure.

Reference: #37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants