search

zytedata / zyte-common-items

Contains the common item definitions used in Zyte.
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

Ignore non-HtmlElement nodes when parsing descriptions #79

Closed Gallaecio closed 7 months ago

Gallaecio commented 7 months ago

Reported by @seagatesoft.

codecov-commenter commented 7 months ago

Codecov Report

Merging #79 (57080d2) into main (56e9709) will not change coverage. The diff coverage is 0.00%.

:exclamation: Current head 57080d2 differs from pull request most recent head 036cc71. Consider uploading reports for the commit 036cc71 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #79 +/- ## ===================================== Coverage 0.00% 0.00% ===================================== Files 11 11 Lines 1766 1768 +2 ===================================== - Misses 1766 1768 +2 ``` | [Files](https://app.codecov.io/gh/zytedata/zyte-common-items/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [zyte\_common\_items/processors.py](https://app.codecov.io/gh/zytedata/zyte-common-items/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-enl0ZV9jb21tb25faXRlbXMvcHJvY2Vzc29ycy5weQ==) | `0.00% <0.00%> (ø)` | |
Gallaecio commented 7 months ago

@wRAR After I created this PR, I thought came to me: Could it be more helpful to raise an exception that indicates the (unsupported) type of node received rather than silently ignoring it? The current PR could lead a developer to believe that things do not work (e.g. due to some bug), rather than they are passing the wrong selector. Specially since it might not be obvious that we are only picking the first selector of a SelectorList (which I am now wondering if it is the right approach for description processors).

wRAR commented 7 months ago

Not 100% sure but it looks better to throw an exception, yeah.