-
Notifications
You must be signed in to change notification settings - Fork 19
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
Handle custom attributes received in the API response. #213
Conversation
Requires zytedata/zyte-common-items#100 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 97.69% 97.86% +0.16%
==========================================
Files 14 14
Lines 1564 1590 +26
Branches 330 337 +7
==========================================
+ Hits 1528 1556 +28
+ Misses 15 14 -1
+ Partials 21 20 -1
|
(some test failures related to field overrides) |
I’ll handle the update of
|
scrapy_zyte_api/providers.py
Outdated
k: ( | ||
dict(v) | ||
if isinstance(v, frozenset) | ||
else list(v) if isinstance(v, tuple) else v | ||
) |
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.
this code seems to be copy-pasted - time to add a helper function? Is it something like undo_make_hashable?
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.
Yeah.
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.
Done, and now I think both functions need tests. We don't actually test what we receive in mockserver requests.
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.
Makes sense, let's add a few tests!
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.
The code looks good. +1 to merge after CI is fixed, and some very basic tests are added for the hash/unhash functions.
We should release zyte-common-items before merging this I think. |
@wRAR feel free to merge when you think it's ready. |
No description provided.