Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Restore _convert_item_to_dict Functionality in ItemValidationPipeline for Compatibility #457

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Restore _convert_item_to_dict Functionality in ItemValidationPipeline for Compatibility #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f075b1
add _convert_item_to_dict method support for backward compatibility
arslansherazi Oct 14, 2024
eb136b9
add test case
arslansherazi Oct 14, 2024
ab9ab5b
add empty line
arslansherazi Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions spidermon/contrib/scrapy/pipelines.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,7 @@ def process_item(self, item, _):
# No validators match this specific item type
return item

item_adapter = ItemAdapter(item)
Copy link
Member

Choose a reason for hiding this comment

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

issue: item_adapter is needed later for self._add_errors_to_item(item_adapter, errors) below.

suggestion: Maybe a test case could be added to check that case.

That's probably why _convert_item_to_dict() was removed. We needed the item adapter later, which that method discarded otherwise.

I think the removal of _convert_item_to_dict() isn't necessarily a backward-incompatible change, as developers shouldn't depend on internal methods. We could restore it, but this pipeline won't use it (as it needs a reference to the adapter object). Or it might use it with some modifications, but at that point, developers will have to refactor their ItemValidationPipeline extensions to accommodate those modifications, too.

@arslansherazi, could you show us some examples of how this pipeline was extended so we can see how to support that case?

item_dict = item_adapter.asdict()
item_dict = self._convert_item_to_dict(item)
self.stats.add_item()
self.stats.add_fields(len(item_dict.keys()))
for validator in validators:
Expand Down Expand Up @@ -158,3 +157,15 @@ def _add_error_stats(self, errors):
for message in messages:
self.stats.add_field_error(field_name, message)
self.stats.add_item_with_errors()

@staticmethod
def _convert_item_to_dict(item: Item) -> dict:
"""
Convert a Scrapy item to a dict.

:param item: Scrapy item
:returns: The item as a dict
"""
item_adapter = ItemAdapter(item)
item_dict = item_adapter.asdict()
return item_dict
8 changes: 8 additions & 0 deletions tests/contrib/scrapy/test_pipelines.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,11 @@ def test_add_errors_to_item_prefilled(self):
"prefilled",
"some_message",
]

@staticmethod
def test_convert_item_to_dict(self):
test_item = TestItem(
{"url": "http://example.com", "error_test": "error_message", "title": "test title"}
)
item_dict = ItemValidationPipeline._convert_item_to_dict(test_item)
assert item_dict == {"url": "http://example.com", "error_test": "error_message", "title": "test title"}