Consider None as return value from pre-processor rather than assume mutation #819
Conversation
| assert schema.load({'value': 3}) is None | ||
| schema = PreSchema() | ||
| assert schema.dump({'value': 3}) == {} | ||
| with pytest.raises(ValidationError) as excinfo: |
There was a problem hiding this comment.
I feel weird about this test.
Returning None from pre_dumppre_load does not make much sense.
There was a problem hiding this comment.
Do you mean pre_load?
Agreed that it doesn't make sense for users to do that, but I think this is the expected behavior.
There was a problem hiding this comment.
Yes, pre_load, sorry.
The test could be catching a ValidationError for some other reason, but there is no way to narrow it without testing private interface, so we can keep it this way.
lafrech
force-pushed
the
dev_347_decorator_returns_none
branch
from
e0f3813
to
eba00e5
Compare
lafrech
force-pushed
the
dev_347_decorator_returns_none
branch
from
eba00e5
to
3b3b83a
Compare
|
Doc updated and PR rebased. |
lafrech
changed the title
| * Clean up code for schema hooks (:issue:`814`). Thanks :user:`taion`. | ||
| * Minor performance improvement from simplifying ``utils.get_value`` (:issue:`811`). Thanks again :user:`taion`. | ||
| * Add ``require_tld`` argument to ``fields.URL`` (:issue:`749`). Thanks | ||
| - Clean up code for schema hooks (:issue:`814`). Thanks :user:`taion`. |
There was a problem hiding this comment.
Thanks for making these consistent.
| In marshmallow 2.x, ``None`` returned by a pre or post-processor is interpreted as "the data was mutated". In marshmallow 3.x, the return value is considered as processed data even if it is ``None``. | ||
|
|
||
| Processors that mutate the data should be updated to also return it. | ||
|
|
There was a problem hiding this comment.
Would be nice to have a code example of how to update existing code, but that doesn't have to block merging of this PR.
|
|
||
| Other changes: | ||
|
|
||
| - *Backwards-incompatible*: Pre/Post-processors must return modified data. |
There was a problem hiding this comment.
This should be in 3.0.0b12. I'll fix this.
There was a problem hiding this comment.
Right. I think it got screwed up during the rebase and I didn't notice.
Commit 43e8b466dd9e3bf1f1d1f4b43d143f42afcddd79 of marshmallow broke our code See also: marshmallow-code/marshmallow#347 marshmallow-code/marshmallow#819 https://marshmallow.readthedocs.io/en/3.0/upgrading.html#pre-post-processors-must-return-modified-data
Fixes #347
TODO: