marshmallow icon indicating copy to clipboard operation
marshmallow copied to clipboard

API docs for `fields.Field.allow_none` and `fields.Field.load_default` are slightly confusing / wrong

Open grmpfhmbl opened this issue 2 years ago • 2 comments

I find the API docs for allow_none and load_default in the fields.Field class slightly confusing / incomplete.

The docs state for allow_none that it defaults to True if it's unset and load_default is None

https://github.com/marshmallow-code/marshmallow/blob/1ddf058fd91ba5e04d7ecbe1751fa9d3ba891ebe/src/marshmallow/fields.py#L99-L101

This kind of contradicts the docs (and as I later found type hints) of load_default. It mentions no default value but states it may be a value or callable - nothing about it being None. I then (wrongly) assumed it defaults to None and that if neither of the parameters are provided allow_none will default to True.

https://github.com/marshmallow-code/marshmallow/blob/1ddf058fd91ba5e04d7ecbe1751fa9d3ba891ebe/src/marshmallow/fields.py#L83-L84

Turns out that assumption was wrong. I got an error message "Field may not be null." and had to read the code to understand that load_default defaults to util.Missing(). On explicitly setting allow_none = True when creating Field() everything worked as expected.

My suggestion is to explicitly mention in the docs for allow_none that it will default to False (or None?) when neither parameter is set and mention the default value of load_default in its docs. Alternatively / additionally you might want to either change the type hint on load_default to include None, or change

https://github.com/marshmallow-code/marshmallow/blob/1ddf058fd91ba5e04d7ecbe1751fa9d3ba891ebe/src/marshmallow/fields.py#L210

to include the load_default == missing_ case. But that probably changes the behaviour of the class in a breaking way, so I'm not sure if you want to opt for that.

grmpfhmbl avatar Aug 26 '22 11:08 grmpfhmbl

The type hint on load_default says Any. I'm not sure what we can change here.

The logic is that allow_none defaults to False unless load_default is None, in which case allow_none defaults to True (because the intent of the user is obviously to allow None in this case).

Perhaps the docstring for allow_none could be rephrased this way, with the usual case first and that specific case mentioned as an exception.

Overall, I think the doc is fine. What tricked you is that load_default does not default to None but to the internal missing singleton. But you might not have gone that far if the docstring for allow_none was clearer, I suppose.

lafrech avatar Aug 26 '22 12:08 lafrech

The type hint on load_default says Any. I'm not sure what we can change here.

Ups... my apologies. Not sure where I was looking when I wrote the type hint sentence. :-(

Overall, I think the doc is fine. What tricked you is that load_default does not default to None but to the internal missing singleton. But you might not have gone that far if the docstring for allow_none was clearer, I suppose.

Very likely. If that pitfall was mentioned at allow_none I likely would have set it to True explicitly right away.

grmpfhmbl avatar Aug 26 '22 14:08 grmpfhmbl