marshmallow
marshmallow copied to clipboard
API docs for `fields.Field.allow_none` and `fields.Field.load_default` are slightly confusing / wrong
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.
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.
The type hint on
load_default
saysAny
. 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 toNone
but to the internalmissing
singleton. But you might not have gone that far if the docstring forallow_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.