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

Change OpenAPIConverter into OpenAPIConverterMixin #488

Closed
wants to merge 2 commits into from
Closed

Conversation

lafrech
Copy link
Member

@lafrech lafrech commented Aug 31, 2019

What I suggested in #478 (comment).

This makes it easier to subclass MarshmallowPlugin.

Copy link
Member

@sloria sloria left a comment

Choose a reason for hiding this comment

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

I'm good with this. It makes MarshmallowPlugin quite fat, but I think it's worth it for the ease of extensibility.

@sloria
Copy link
Member

sloria commented Aug 31, 2019

An alternative approach would be to set the converter as a class member on MarshmallowPlugin.

class MarshmallowPlugin(BasePlugin):
    Converter = OpenAPIConverter

The plugin can be extended like so:

class MyMarshmallowPlugin(MarshmallowPlugin):
    class Converter(MarshmallowPlugin.Converter):
        def property2parameter(self, ...):
            ...

wdyt?

@lafrech
Copy link
Member Author

lafrech commented Sep 1, 2019

Thanks for the doc update.

Mixin vs. class member is a matter of taste. Both work.

With class member, subclassing just takes one more line. Neglectable.

I have no issue with the plugin class being fat since the mixin is in another file, like if it was another class.

(I don't know if you ever looked at flask-rest-api. I separated the features using mixins. I'm not sure those are real mixins like mixins are meant for, it's hard because the features are not strictly independent. But it makes for a nice split of the code.)

Perhaps the mixin here makes sense because it avoids passing the schema_name_resolver from the plugin to the converter, or fetching the refs in the converter from the plugin, and so on. The separation is not clear enough to do things nicely with a separate class.

@sloria
Copy link
Member

sloria commented Sep 1, 2019

I have no strong preference. Things can get confusing when inheritance chains get long with lots of mixins (at work we're considering running with the nested class approach in our REST framework), but that's not a problem here.

(I don't know if you ever looked at flask-rest-api. I separated the features using mixins.

werkzeug organizes things this way, too: https:/pallets/werkzeug/blob/9844e0447e63366273c8b9f9f2400e19b45532d1/src/werkzeug/wrappers/request.py#L9-L16 . It's a nice way to split up the code.

So long as you're OK with publicizing all these methods, let's go ahead with this.

@lafrech
Copy link
Member Author

lafrech commented Sep 2, 2019

So long as you're OK with publicizing all these methods, let's go ahead with this.

The point is to make subclassing easier. I suppose allowing users to subclass means we're considering it public, either way (be it mixin or class member).

Should we remove the docstrings in openapi.py and field_converter.py saying the module is private API?

@sloria
Copy link
Member

sloria commented Sep 2, 2019

I guess the class member way makes the methods "semi-private", as in you can still override them if you know what you're doing, but the converter class itself can remain documented as private/subject to change.

Should we remove the docstrings in openapi.py and field_converter.py saying the module is private API?

We could just remove :inherited-members: from the apispec.ext.marshmallow autodoc in api_ext.py. Your call on this.

lafrech and others added 2 commits September 2, 2019 17:17
* Document inherited members of MarshmallowPlugin
* Undoc members of openapi and field_converter modules
* Minor copy edits
@lafrech
Copy link
Member Author

lafrech commented Sep 3, 2019

Closing in favor of #493.

@lafrech lafrech closed this Sep 3, 2019
@lafrech lafrech deleted the openapi_mixin branch September 3, 2019 07:27
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