scrapinghub / spidermon

Scrapy Extension for monitoring spiders execution.
https://spidermon.readthedocs.io
BSD 3-Clause "New" or "Revised" License
530 stars 96 forks source link

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

Closed rochamatcomp closed 7 months ago

rochamatcomp commented 8 months ago

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 commented 8 months ago

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?

rennerocha commented 8 months ago

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 commented 7 months ago

The two failed tests are fixed in #432.

VMRuiz commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Sorry, my previous comment was aim to @rennerocha

codecov[bot] commented 7 months ago

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 ```diff @@ 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.