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

Add a Geolocation dependency. #167

Merged
merged 4 commits into from
Feb 1, 2024
Merged

Add a Geolocation dependency. #167

merged 4 commits into from
Feb 1, 2024

Conversation

wRAR
Copy link
Contributor

@wRAR wRAR commented Jan 29, 2024

No description provided.

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Merging #167 (85d11dc) into main (ba047c8) will increase coverage by 0.17%.
Report is 12 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
+ Coverage   98.25%   98.43%   +0.17%     
==========================================
  Files          11       11              
  Lines         862      892      +30     
==========================================
+ Hits          847      878      +31     
+ Misses         15       14       -1     
Files Coverage Δ
scrapy_zyte_api/__init__.py 100.00% <100.00%> (ø)
scrapy_zyte_api/_annotations.py 100.00% <100.00%> (ø)
scrapy_zyte_api/providers.py 97.05% <100.00%> (+0.28%) ⬆️

... and 1 file with indirect coverage changes

@attrs.define
class MyPageObject(BasePage):
product: Product
geolocation: Annotated[Geolocation, "DE"]
Copy link
Member

@kmike kmike Jan 30, 2024

Choose a reason for hiding this comment

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

I haven't checked it, but wonder if something like this would work:

class _Geolocation:
    pass

def Geolocation(geolocation: str):
    return Annotated[_Geolocation, geolocation]

@attrs.define
class MyPageObject(ProductPage):
    geolocation: Geolocation("DE")

Where does it break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: Invalid type comment or annotation [valid-type] from mypy, though the runtime code works.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Let's not handle it here then. This mypy issue seems relevant: python/mypy#13643 (with a suggested workaround).

scrapy_zyte_api/providers.py Outdated Show resolved Hide resolved
@@ -150,6 +154,10 @@ async def __call__( # noqa: C901
cls_stripped = strip_annotated(cls)
assert isinstance(cls_stripped, type)
kw = item_keywords.get(cls_stripped)
if cls_stripped is Geolocation and is_typing_annotated(cls):
item = AnnotatedResult(Geolocation(), cls.__metadata__) # type: ignore[attr-defined]
results.append(item)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a need to update cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand it the purpose of the cache is to skip requesting things, and we don't request Geolocation, but also we don't want to remove it from to_provide like it would be done if it's found in the cache. On the other hand I don't know in which cases is the cache currently used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also is it possible to request a Product without a Geolocation, cache it and then get that instance when the provider code is called again with a Geolocation, thus getting a wrong one?

@kmike kmike merged commit 4adedc8 into main Feb 1, 2024
19 checks passed
@wRAR wRAR deleted the dep-geolocation branch April 24, 2024 06:12
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.

None yet

3 participants