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

Change format of content of _validation field (#425) #431

Conversation

rochamatcomp
Copy link
Contributor

The content of _validation field must to be the string representation of a Python dict instead defaultdict when SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS setting is True.

@VMRuiz
Copy link
Collaborator

VMRuiz commented Feb 8, 2024

Thanks @rochamatcomp for the PR!

@curita  @fcanobrash Are you aware of any other tool that may need to be updated to adapt to this new format?


for field_name, messages in errors.items():
errors_field_instance[field_name] += messages

# change defaultdict to dict for errors_field_instance
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick This comment is redundant.

@rennerocha
Copy link
Collaborator

Thanks @rochamatcomp for the PR!

@curita @fcanobrash Are you aware of any other tool that may need to be updated to adapt to this new format?

@VMRuiz given that a defaultdict is a subclass of dict, anything you were able to do with the content of _validation key should work fine, except if we try to access a key that is missing, which is not an expected use case. Generally in the user's code we don´t know the content of this key and just read it as it is (not trying to check for different values).

@rochamatcomp
Copy link
Contributor Author

The two failed tests are fixed in #432.

@VMRuiz
Copy link
Collaborator

VMRuiz commented Feb 9, 2024

Thanks @rochamatcomp for the PR!
@curita @fcanobrash Are you aware of any other tool that may need to be updated to adapt to this new format?

@VMRuiz given that a defaultdict is a subclass of dict, anything you were able to do with the content of _validation key should work fine, except if we try to access a key that is missing, which is not an expected use case. Generally in the user's code we don´t know the content of this key and just read it as it is (not trying to check for different values).

Notice that in your issue you said:

To make this field valid as JSON, we should have it as a list of objects such as:
'_validation': [ {'author_url': ['Invalid URL'] }]

which would require to convert _validation type from defaultdict to list. However, the current solution converts from defaultdict to dict. This is way I was asking about possible conflicts.

Is @rochamatcomp approach what you were expecting?

@rochamatcomp
Copy link
Contributor Author

Thanks @rochamatcomp for the PR!
@curita @fcanobrash Are you aware of any other tool that may need to be updated to adapt to this new format?

@VMRuiz given that a defaultdict is a subclass of dict, anything you were able to do with the content of _validation key should work fine, except if we try to access a key that is missing, which is not an expected use case. Generally in the user's code we don´t know the content of this key and just read it as it is (not trying to check for different values).

Notice that in your issue you said:

To make this field valid as JSON, we should have it as a list of objects such as:
'_validation': [ {'author_url': ['Invalid URL'] }]

which would require to convert _validation type from defaultdict to list. However, the current solution converts from defaultdict to dict. This is way I was asking about possible conflicts.

Is @rochamatcomp approach what you were expecting?

I'm expecting change from defaultdict(list, {'_validation': [{'author_url': ['Invalid URL']}]}) to {'_validation': [{'author_url': ['Invalid URL']}]}

The list inside the default/dict don't change.

@VMRuiz
Copy link
Collaborator

VMRuiz commented Feb 15, 2024

Sorry, my previous comment was aim to @rennerocha

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5214606) 79.39% compared to head (12f63bc) 79.39%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #431   +/-   ##
=======================================
  Coverage   79.39%   79.39%           
=======================================
  Files          76       76           
  Lines        3223     3223           
  Branches      534      534           
=======================================
  Hits         2559     2559           
  Misses        593      593           
  Partials       71       71           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The content of _validation field must to be the string representation of a Python dict instead defaultdict when SPIDERMON_VALIDATION_ADD_ERRORS_TO_ITEMS setting is True.
@rochamatcomp rochamatcomp force-pushed the change_format_content_validation_field branch from 94d5524 to 12f63bc Compare February 16, 2024 16:51
@VMRuiz VMRuiz merged commit ab611d6 into scrapinghub:master Mar 7, 2024
7 checks passed
@rochamatcomp rochamatcomp deleted the change_format_content_validation_field branch March 13, 2024 01:14
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.

3 participants