Closed kishan3 closed 8 months ago
Hi @asadurski @lopuhin please review. Thanks.
Thanks @lopuhin @Gallaecio I made the suggested changes can you please check?
Merging #62 (c1fc711) into main (d6765ee) will not change coverage. The diff coverage is
0.00%
.:exclamation: Current head c1fc711 differs from pull request most recent head f716be8. Consider uploading reports for the commit f716be8 to get more accurate results
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
@kishan3 there looks to be a build failing due to docstring issue https://github.com/zytedata/zyte-common-items/actions/runs/6720157548/job/18263131704?pr=62#step:5:83
@kishan3 there looks to be a build failing due to docstring issue https://github.com/zytedata/zyte-common-items/actions/runs/6720157548/job/18263131704?pr=62#step:5:83
Thanks @lopuhin I fixed it now.
Thanks @Gallaecio for the detailed review. I updated the code as per suggestions.
Looks good to me. +1 to merge once this new type is covered in https://docs.zyte.com/zyte-api/usage/extract.html#request-fields (no strong opinion against merging before that, though).
Thanks, will wait for this new type to appear in docs. Is there a place I can track it's progress or maybe I can contribute to adding it?
Is there a place I can track it's progress or maybe I can contribute to adding it?
@lopuhin Probably knows best when it will be available in Zyte API. Then you can add it to docs.zyte.com if you want.
I have just realized that we don’t really need to wait for Zyte API automatic extraction support, since we already have classes here that are not yet supported. What we should aim for is to have the schema in https://docs.zyte.com/zyte-data/schemas/index.html, which is what the classes here are meant to follow.
Thanks @Gallaecio @wRAR!!
Doc: https://docs.google.com/spreadsheets/d/1zSMSjyS0kkVcSTE-iV5U3U2SIP1QMv6PhgMNJC8Qq6M/edit\#gid\=1425038735 Unified schema: https://github.com/scrapinghub/unified-schema/pull/279