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.

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

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

Conversation

arslansherazi
Copy link

@arslansherazi arslansherazi commented Oct 14, 2024

This pull request restores the _convert_item_to_dict functionality in the ItemValidationPipeline, which was available in previous versions but removed in the current release.

For users who extended the ItemValidationPipeline with the spidermen pipeline and relied on the
_convert_item_to_dict function, this removal could break their code. By reintroducing this functionality, we ensure those users can continue using it without needing to refactor their pipelines.

This update provides compatibility for users relying on the older version's behavior, while allowing users of the current version to avoid any issues or breaking changes.

Sample Code:

Screenshot 2024-10-14 at 10 52 59 PM

@arslansherazi arslansherazi changed the title Add convert to item support for older versions Restore convert_item_to_dict Functionality in ItemValidationPipeline for Compatibility Oct 14, 2024
@arslansherazi arslansherazi changed the title Restore convert_item_to_dict Functionality in ItemValidationPipeline for Compatibility Restore _convert_item_to_dict Functionality in ItemValidationPipeline for Compatibility Oct 14, 2024
@VMRuiz VMRuiz requested a review from Gallaecio October 16, 2024 07:45
@@ -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?

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.

2 participants