spidermon icon indicating copy to clipboard operation
spidermon copied to clipboard

Change format of content of _validation field (#425)

Open rochamatcomp opened this issue 1 year ago • 7 comments

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 avatar Feb 08 '24 01:02 rochamatcomp

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 avatar Feb 08 '24 07:02 VMRuiz

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

rennerocha avatar Feb 08 '24 10:02 rennerocha

The two failed tests are fixed in #432.

rochamatcomp avatar Feb 08 '24 11:02 rochamatcomp

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?

VMRuiz avatar Feb 09 '24 08:02 VMRuiz

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.

rochamatcomp avatar Feb 14 '24 12:02 rochamatcomp

Sorry, my previous comment was aim to @rennerocha

VMRuiz avatar Feb 15 '24 10:02 VMRuiz

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 16 '24 16:02 codecov[bot]