spidermon icon indicating copy to clipboard operation
spidermon copied to clipboard

Adding use of ItemAdapter to prevent assumptions of item nature

Open further-reading opened this issue 3 years ago • 3 comments

https://github.com/scrapinghub/spidermon/issues/353

further-reading avatar Aug 12 '22 11:08 further-reading

Codecov Report

Base: 74.28% // Head: 74.62% // Increases project coverage by +0.34% :tada:

Coverage data is based on head (454eefc) compared to base (5020fca). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   74.28%   74.62%   +0.34%     
==========================================
  Files          73       73              
  Lines        3126     3113      -13     
  Branches      487      366     -121     
==========================================
+ Hits         2322     2323       +1     
+ Misses        737      725      -12     
+ Partials       67       65       -2     
Impacted Files Coverage Δ
spidermon/contrib/scrapy/extensions.py 85.41% <100.00%> (+0.15%) :arrow_up:
spidermon/contrib/scrapy/pipelines.py 83.50% <100.00%> (+7.14%) :arrow_up:
spidermon/contrib/validation/jsonschema/formats.py 100.00% <0.00%> (+28.57%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Aug 19 '22 06:08 codecov[bot]

hey! Would it be possible to add tests for it?

kmike avatar Aug 19 '22 06:08 kmike

It's still a work in progress haha, of course there'll be tests. We can hold off on the reviews until its ready.

further-reading avatar Aug 19 '22 08:08 further-reading

Looks good to me.

I would however add itemadapter as a dependency now. Things will work either way because of the dependency with Scrapy, but it makes things more future-proof (e.g. in the unlikely case that Scrapy drops itemadapter as a dependency).

@further-reading Could you please fix the missing dependency in setup.py ?

VMRuiz avatar Oct 06 '22 14:10 VMRuiz

@further-reading Unless you plan on further changes, e.g. https://github.com/scrapinghub/spidermon/pull/358#discussion_r968693785, we can merge as far as I am concerned.

Gallaecio avatar Oct 20 '22 14:10 Gallaecio

@Gallaecio apologies I missed that one. Should be sorted out now

further-reading avatar Oct 20 '22 14:10 further-reading

Thank you!

Gallaecio avatar Oct 20 '22 18:10 Gallaecio