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

add_attribute_function: bind function to OpenAPIConverter instance #498

Merged
merged 4 commits into from Sep 9, 2019

Conversation

@lafrech
Copy link
Member

commented Sep 8, 2019

This achieves the same goal as #497.

It makes things more consistent to have custom attribute functions be bound to the instance like the default ones.

This means all custom functions must accept self (even if they don't use it). I think consistency is worth this (little) inconvenience.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

Alternatives:

  • Don't do anything (don't bind, don't pass self) and let the user declare the function as a method if needed by subclassing the converter. Doing this plus calling add_attribute_function means double work. I don't like that.

  • Only declare static functions in FieldConverterMixin and always pass the converter as keyword arg (or secondary positional, but kwarg is easier to ignore from the custom func):

class FieldConverterMixin:

    def field2property(self, field):
        ret = {}
        for attr_func in self.attribute_functions:
            ret.update(attr_func(field, openapi_converter=self, ret=ret))
        return ret

    @staticmethod
    def field2type_and_format(field, openapi_converter, **kwargs):
        [...]

    @staticmethod
    def field2default(field, **kwargs):
        [...]

def my_custom_funct(field, openapi_converter, **kwargs):
    [...]

converter.add_attribute_function(my_custom_funct)

This way, all signatures are the same.

I'm not sure which interface is the best. But current interface looks awkward to me.

@Bangertm

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

This works for me. I agree that the current interface is awkward and it's better to have a consistent interface. Currently three field types, (Nested, List, and Dict) require access to self. I image that at least some of the custom fields would require it as well.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Yes, and one could also add custom methods not only for custom fields but for custom properties.

In fact, any method potentially needs access to the converter just to know the OAS version.

I'm still hesitant between this PR and the second alternative above (static methods + self/openapi_converter passed as kwargs).

The latter is slightly clearer to the user creating a custom function because the former (this PR) means the user creating a custom function writes a function having self as first positional args while not in a class scope.

def my_custom_func(self, field, **kwargs):  # <-- self, here, while not in class scope
    [...]

ma_plugin.converter.add_attribute_function(my_custom_func)

OTOH, the openapi_converter kwarg might not make much intuitive sense either, as the converter is an internal class very specific to our implementation. Not much more self-explanatory than self. And it's weird to pass the converter instance as a kwarg to the methods declared in the class.

def my_custom_func(field, openapi_converter, **kwargs):  # <-- openapi_converter here
    [...]

ma_plugin.converter.add_attribute_function(my_custom_func)

Maybe I'm overthinking it.

I'll probably merge this. Just need to update docs/changelog.

Meanwhile, thoughts welcome. I can rework for the static functions alternative.

@Bangertm

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

One additional alternative would be to make all static methods but instead of passing the openapi_converter instance just pass the things that we think the the static methods needs as keyword arguments i.e. openapi_version, resolve_nested_schema and field2parameters. I think that providing self is the better way to go.

At the end of the day we are saving users having to create their logic in a subclass. So by requiring them to add a self positional argument they they have to write the exact same method but without the need to subclass.

@lafrech lafrech merged commit 423afe8 into dev Sep 9, 2019

10 checks passed

marshmallow-code.apispec Build #20190909.2 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 bind_add_attribute_function branch Sep 9, 2019

@@ -279,17 +279,21 @@ method. Continuing from the example above:

.. code-block:: python
def my_custom_field2properties(field, **kwargs):
def my_custom_field2properties(self, field, **kwargs):

This comment has been minimized.

Copy link
@sloria

sloria Sep 9, 2019

Member

Maybe rename the first arg to make it more clear? converter?

This comment has been minimized.

Copy link
@lafrech

lafrech Sep 9, 2019

Author Member

Yeah, I was wondering.

The point is that the custom and stock functions look alike. And the doc says the function will be bound. We could stress out bound -> self, or s/self/converter. But converter is not that self-explanatory either.

No strong feeling either way. In any case, it's a trick in trick's clothing.

This comment has been minimized.

Copy link
@sloria

sloria Sep 9, 2019

Member

Fair enough. I've no strong opinion either way. self just looks strange on a function, but yeah..'trick's clothing'. =)

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.