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

Add missing_values parameter to Field #1381

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Add missing_values parameter to Field #1381

wants to merge 1 commit into from

Conversation

sloria
Copy link
Member

@sloria sloria commented Sep 8, 2019

Proves the concept proposed in #713 (comment)

Still needs docs and details, but this is ready for review.

TODO:

  • Respect missing_values on serialization? see discussion
  • Update Field.__repr__ to include missing_values
  • Docs

with pytest.raises(ValidationError, match="required"):
ArtistSchema().load({"album_names": []})

def test_setting_default_missing_values(self, monkeypatch):
Copy link
Member Author

Choose a reason for hiding this comment

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

This test might not be necessary--it's sorta just testing the Python language. Just wanted to make sure this use case was met.

Allows specifying which values are treated as "missing".

Addresses #713
@lafrech
Copy link
Member

lafrech commented Sep 9, 2019

LGTM.

Respect missing_values on serialization?

What would that do? I'm tempted to say no. Do we do anything specific with missing already?

@sloria
Copy link
Member Author

sloria commented Sep 9, 2019

It would make default value get returned for missing values on serialization.

from marshmallow import Schema, fields


class ArtistSchema(Schema):
    name = fields.Str(default="---", missing_values=("",))


sch = ArtistSchema()
print(sch.dump({}))   # =>  {'name': '---'}
print(sch.dump({"name": ""})) # => {'name': ''} now, but maybe should be same as above

Sure, it's not a common use case, but it seems inconsistent to not handle it.

@lafrech
Copy link
Member

lafrech commented Sep 9, 2019

OK, I get it.

Maybe it shouldn't be the same parameter.

IIUC, the primary use case for this is an API for which it is complicated to enforce removal of empty-ish values. There is no reason the same constraint would apply to the serialized form of the data.

But the database may have its own constraint. When using SQL, I already felt the need to treat None as missing to avoid returning null (#229 (comment)) and your proposal would solve that. But I didn't want to treat null as missing when deserializing.

Symmetry would recommend default_values, although the name is a bit misleading.

@sloria
Copy link
Member Author

sloria commented Sep 9, 2019

That's a good point; the client inputs for "no data" may be different from the database's representation.

default_values is indeed misleading. If we go with #778 , we could do load_default_values and dump_default_values, I guess. Maybe we defer the serialization param for now.

@lafrech
Copy link
Member

lafrech commented Apr 8, 2021

default_values is indeed misleading. If we go with #778 , we could do load_default_values and dump_default_values, I guess. Maybe we defer the serialization param for now.

#1742 is about to close #778.

We could then change the names here. And do the serialization part in here or another PR.

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