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

Documenting callable default #196

Closed
lafrech opened this issue Apr 5, 2018 · 4 comments
Closed

Documenting callable default #196

lafrech opened this issue Apr 5, 2018 · 4 comments

Comments

@lafrech
Copy link
Member

lafrech commented Apr 5, 2018

When a default/missing value is a callable, it is called when generating the documentation.

    if default is not marshmallow.missing:
        if callable(default):
            ret['default'] = default()
        else:
            ret['default'] = default

This can be wrong if the value of the callable depends on the moment it is called. Typically, if it is dt.datetime.now. There should be an opt-out for this allowing to specify an alternative text to write in the docs.

Not sure how to do that. Maybe a default_doc attribute in the Schema that would take precedence over default/missing.

@sloria
Copy link
Member

sloria commented Apr 10, 2018

Ah, that makes sense. Good catch. I think your proposed default_doc parameter is a fine approach.

@lafrech
Copy link
Member Author

lafrech commented Apr 10, 2018

There is no way to document a dynamic default: OAI/OpenAPI-Specification#554.

What we should do is allow the user to opt-out of default documenting. Then he can either specify the default behaviour in the description or add a x-something attribute.

So we need an attribute meaning "don't document default". default_doc is not really a good name for that.

From the discussion in OAI/OpenAPI-Specification#554, datetime is the only common use case. Not documenting callable defaults is not really an option, as we'll miss dict, list and such.

@lafrech
Copy link
Member Author

lafrech commented Apr 10, 2018

On second thought, since a callable could be anything, it could be smarter to not document them. There is no reason the value returned by the callable at documentation time makes sense in this context.

An exception to this would be basic types that are provided as callables to avoid the "same list instance in every field instance" issue. Those are common enough to justify an exception to the rule.

So

    if default is not marshmallow.missing:
        if callable(default):
            if default in (dict, list, set,...):  # How to get an exhaustive list?
                ret['default'] = default()
            else:
                # Don't assume anything and let the user add a note in the description
                pass
        else:
            ret['default'] = default

We could add a default_doc to allow the user to specify a static default manually to the docs if the callable happens to always return the same content but is not a known collection class. We can even accept a callable here.

    EMPTY_COLLECTION_CALLABLES = (dict, list, set,...)  # How to get an exhaustive list?

    if default_doc:
        if callable(default_doc):
            ret['default'] = default_doc()
        else:
            ret['default'] = default_doc
    elif default is not marshmallow.missing:
        if callable(default) and default in EMPTY_COLLECTION_CALLABLES:
                ret['default'] = default()
        else:
            ret['default'] = default

@lafrech
Copy link
Member Author

lafrech commented Jul 16, 2018

I'd like to tackle this.

Using the callable's return value to fill the doc default is wrong, so removing this would be a bugfix. Besides, it is not tested, so no test broken. Edit: My bad, it is tested in test_ext_marshmallow.py.

I wanted to make an exception for list and dict and their subclasses, but even including subclasses is wrong, as some subclasses might do weird stuff. But we could want to include OrderedDict,... and things can get complicated. I'm not sure it is such a good idea to make an exception, after all.

However, it would be nice to have another meta attribue to allow passing a default manually. This one could also be used when dict or list is used as default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants