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

Fix documentation of default from missing with marshmallow 3 #490

Merged
merged 4 commits into from Sep 3, 2019

Conversation

@lafrech
Copy link
Member

commented Aug 31, 2019

Addresses #342.

It sucks not being able to document default values automatically, especially since it would probably work most of the times, but I don't see a safe way to do it.

Without this feature removal, we may pass to the spec dict objects that are not json serializable, which will trigger an exception when serializing the spec file.

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

OTOH, in most cases, dumping missing is perfectly fine, so removing automatic default documentation would be a regression.

This. As a user, I want apispec to work mostly transparently, i.e. without having to modify my app code.

Maybe we could handle errors better to lead the user toward using doc_default when it's necessary, but I don't think we should degrade the 90% use case.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

Well, we could try to dump the missing value with the Field. Let's give it a try.

I don't know if we should document the missing value of Nested fields or container fields with Nested inside. The examples I found for default values are "scalar" values like integers or strings but I'm not sure a dict is allowed. Anyway, that's nothing new. Same applies to MA2.

@lafrech lafrech changed the title Don't autodocument default from missing with marshmallow 3 WIP - Fix documentation of default from missing with marshmallow 3 Sep 2, 2019

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

I just pushed a new commit to serialize missing with the field.

It has a few smells:

  • Accesses underscored _serialize method (no big deal).
  • Can't pass attr and obj parameters of _serialize. This shouldn't be an issue in most cases as those are almost never used. In fact, the only uses I know of are Function and Method (I already commented about this in marshmallow-code/marshmallow#1046) and those are dump_only.

We should probably only document missing for fields that are not dump_only. It doesn't make sense anyway, does it?

Also, I'd need to add proper tests to show it works with a field where this matters, such as DateTime.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

Not quite.

Indeed we shouldn't document default on a dump_only field.

However, Function and Method may not be dump_only and in this case, the code is likely to crash. But maybe not, if the method can take None as a parameter.

Maybe we could handle errors better to lead the user toward using doc_default when it's necessary

So shall we call _serialize, catch Exception and return a message telling the user to use doc_default? The result may even be wrong without an exception being raised, so this wouldn't be 100% sure. But admittedly, those would be corner cases. Perhaps a note in the docs would be sufficient.

Or rather than passing None, could we pass some sort of proxy object as attr and obj that would raise an exception when used? For obj, we could override getter methods, etc. But for attr...

@sloria

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

For now, I think documentation may be the best course.

@lafrech lafrech force-pushed the rework_default_ma3 branch 2 times, most recently from ac54b59 to 216afa4 Sep 2, 2019

@lafrech lafrech force-pushed the rework_default_ma3 branch 2 times, most recently from a6590ea to fdf1a6e Sep 3, 2019

@lafrech lafrech force-pushed the rework_default_ma3 branch from fdf1a6e to d924a44 Sep 3, 2019

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

I added a piece of doc about MarshmallowPlugin.

Feedback welcome.

Indeed we shouldn't document default on a dump_only field.

Maybe not but a field with dump_only and missing is wrong in marshmallow anyway, and we generally don't try to validate user inputs in apispec, so I removed the corresponding case from this PR.

Note: default in _VALID_PROPERTIES is useless because it can't be in metadata as it is a reserved marshmallow attribute. I thought of removing it from the list but things might change in marshmallow-code/marshmallow#1350 (MA 4) so I left it there for now, it is harmless.

@sloria
sloria approved these changes Sep 3, 2019
@sloria

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Go ahead and merge when you think this is ready.

@lafrech lafrech changed the title WIP - Fix documentation of default from missing with marshmallow 3 Fix documentation of default from missing with marshmallow 3 Sep 3, 2019

@lafrech lafrech merged commit 7016ebc into dev Sep 3, 2019

10 checks passed

marshmallow-code.apispec Build #20190903.6 succeeded
Details
marshmallow-code.apispec (tox_linux docs) tox_linux docs succeeded
Details
marshmallow-code.apispec (tox_linux lint) tox_linux lint succeeded
Details
marshmallow-code.apispec (tox_linux py35-marshmallow2) tox_linux py35-marshmallow2 succeeded
Details
marshmallow-code.apispec (tox_linux py35-marshmallow3) tox_linux py35-marshmallow3 succeeded
Details
marshmallow-code.apispec (tox_linux py36-marshmallow2) tox_linux py36-marshmallow2 succeeded
Details
marshmallow-code.apispec (tox_linux py36-marshmallow3) tox_linux py36-marshmallow3 succeeded
Details
marshmallow-code.apispec (tox_linux py37-marshmallow2) tox_linux py37-marshmallow2 succeeded
Details
marshmallow-code.apispec (tox_linux py37-marshmallow3) tox_linux py37-marshmallow3 succeeded
Details
marshmallow-code.apispec (tox_linux py37-marshmallowdev) tox_linux py37-marshmallowdev succeeded
Details

@lafrech lafrech deleted the rework_default_ma3 branch Sep 3, 2019

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@sloria, @Bangertm, anything else you'd like to add to 3.0 ?

@sloria

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

nothing from me

@Bangertm

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

Been checked out the past few days, but I like the recent changes. The only other thing I have been thinking about is extracting the logic for finding Schemas in OpenAPI objects from MarshmallowPlugin and/or improving the documentation of those methods.

resolve_parameters, resolve_schema_in_request_body, resolve_schema and OpenAPIConverter.resolve_schema_dict all have to do with searching for schemas within various OpenAPI objects and then converting them via OpenAPIConverter. It might be convenient if all this logic was implemented in a mixin class so that MarshmallowPlugin was left with just the plugin methods and a couple of helpers.

It would also be convenient if the documentation for these methods was a little more clear about the expected structure of the input and where schemas could go. Part of the rational for pulling all the logic out would be to improve documentation and understandably, but we could also just improve the documentation of the methods as they are now.

@sloria

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

resolve_parameters, resolve_schema_in_request_body, resolve_schema and OpenAPIConverter.resolve_schema_dict all have to do with searching for schemas within various OpenAPI objects and then converting them via OpenAPIConverter. It might be convenient if all this logic was implemented in a mixin class so that MarshmallowPlugin was left with just the plugin methods and a couple of helpers.

To follow the pattern in #493, we could do

class MarshmallowPlugin(BasePlugin):
    SchemaResolver = SchemaResolver

class MyMarshmallowPlugin(MarshmallowPlugin):
    class SchemaResolver(MarshmallowPlugin.SchemaResolver):
        def resolve_parameters(...):
            ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.