Skip to content

Conversation

@further-reading
Copy link
Contributor

@further-reading further-reading self-assigned this Aug 12, 2022
@further-reading further-reading marked this pull request as draft August 12, 2022 11:29
@VMRuiz VMRuiz requested a review from rennerocha August 18, 2022 09:01
@VMRuiz VMRuiz force-pushed the 353-use-itemadapter branch from d1c7150 to 82b8a5c Compare August 19, 2022 06:48
@VMRuiz VMRuiz requested review from Gallaecio, kmike and wRAR August 19, 2022 06:50
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

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

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%) ⬆️
spidermon/contrib/scrapy/pipelines.py 83.50% <100.00%> (+7.14%) ⬆️
spidermon/contrib/validation/jsonschema/formats.py 100.00% <0.00%> (+28.57%) ⬆️

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

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

@kmike
Copy link
Member

kmike commented Aug 19, 2022

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

@further-reading
Copy link
Contributor Author

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

@rennerocha rennerocha added this to the 1.18.0 milestone Sep 12, 2022
@further-reading further-reading changed the title [WIP] Adding use of ItemAdapter to prevent assumptions of item nature Adding use of ItemAdapter to prevent assumptions of item nature Sep 12, 2022
@further-reading further-reading marked this pull request as ready for review September 12, 2022 15:26
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

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).

@VMRuiz
Copy link
Collaborator

VMRuiz commented Oct 6, 2022

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 ?

@Gallaecio
Copy link
Member

Gallaecio commented Oct 20, 2022

@further-reading Unless you plan on further changes, e.g. #358 (comment), we can merge as far as I am concerned.

@further-reading
Copy link
Contributor Author

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

@Gallaecio Gallaecio merged commit f6406b7 into scrapinghub:master Oct 20, 2022
@Gallaecio
Copy link
Member

Thank you!

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.

7 participants