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

Support for custom type dispatcher #48

Conversation

smarzola
Copy link
Contributor

Is not uncommon to extend colander by defining new types, but cornice.ext.swagger did not provide an easy way to support custom types (in fact you had to monkey patch the library to achieve this).

I've addressed this issue by allowing a custom type dispatcher to be passed to the Swagger constructor.

@coveralls
Copy link

coveralls commented Jan 22, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d6b529b on OvalMoney:feat-extensible-type-convertion-dispatcher into 67002a5 on Cornices:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d6b529b on OvalMoney:feat-extensible-type-convertion-dispatcher into 67002a5 on Cornices:master.

@@ -276,14 +283,18 @@ class DefinitionHandler(object):

json_pointer = '#/definitions/'

def __init__(self, ref=0):
def __init__(self, ref=0, typ_dispatcher=TypeConversionDispatcher()):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a good reason to have to overwrite the whole TypeConversionDispatcher class? Can't we just pass a dict mapping the user defined cornice types to a callable implementing the cornice_swagger.converters.schema.TypeConverter interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that I needed to change the behaviour of the TypeConversionDispatcher and not only add some custom type to fit our use case. Since our custom types always inherits from colander.String, I've adopted a solution like this:

class TypeConversionDispatcher(object):
    converters = defaultdict(lambda: StringTypeConverter)

    converters.update({
        colander.Boolean: BooleanTypeConverter,
        colander.Date: DateTypeConverter,
        colander.DateTime: DateTimeTypeConverter,
        colander.Float: NumberTypeConverter,
        colander.Integer: IntegerTypeConverter,
        colander.Mapping: ObjectTypeConverter,
        colander.Sequence: ArrayTypeConverter,
        colander.String: StringTypeConverter,
        colander.Time: TimeTypeConverter,
    })
...

I do realize that's not a common use case, so it's ok for me to go with a dict of type:converter

Copy link
Collaborator

@gabisurita gabisurita Jan 22, 2017

Choose a reason for hiding this comment

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

Hmm, I see. But I still think that passing the dispatcher is a bit extreme.

One solution that i think is a little more simple is to have custom_type_converters={} that update the default converters and default_type_converter=None that is used when we don't have an entry for that type instead of raising an exception. Thoughts?

Edit: I would probably use a default converter as well :)

@gabisurita
Copy link
Collaborator

Related to #37

@smarzola
Copy link
Contributor Author

What do you think @gabisurita ?

@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5dee40e on OvalMoney:feat-extensible-type-convertion-dispatcher into 6b3d731 on Cornices:master.

@glasserc
Copy link
Contributor

Obviously @gabisurita knows this package better than I do, but here's my opinion. This PR needs to solve two basic problems. 1. How do we define a type dispatcher? 2. How do we tell the rest of the package to use my custom type dispatcher?

One common solution in Python for problem 1 is to have the developer subclass the existing type dispatcher. In this case, that might look like:

class MyTypeDispatcher(TypeDispatcher):
    converters = dict(TypeDispatcher.converters)
    converters.update({
        colander.Foo: FooConverter,
    })

    def __call__(self, schema_node):
        try:
            super(self, MyTypeConverter).__call__(schema_node)
        except NoSuchConverter:
            return DefaultConverter()(schema_node)

This seems like a pretty lightweight mechanism to define a new type dispatcher that doesn't require threading arguments through the rest of the class structure.

So then we're left trying to solve problem 2. I think the cleanest approach is to take a reference to the type dispatcher class when we construct the CorniceSwagger object. Another approach might be to let the user set CorniceSwagger.type_dispatcher = MyTypeDispatcher. A third approach might be to allow the user to subclass CorniceSwagger and provide their own type_dispatcher attribute or convert_schema method.

Actually, while we're on the subject, we probably don't really need the convert_schema function, right? convert_schema(foo) is just SomeTypeDispatcher()(foo). And actually, why do we keep the TypeDispatcher class instead of just a type_dispatcher object? It seems like there's no reason we would need to create new ones all the time -- they shouldn't have state or anything. I think part of the reason this function exists is because convert_schema is a much more informative name than __call__. Maybe this is a good opportunity to rename __call__ for dispatchers to something like convert(self, schema), or convert_schema or convert_colander or something?

@smarzola
Copy link
Contributor Author

smarzola commented Jan 24, 2017

@glasserc you have kind of described my first approach to the issue. But after discussing with @gabisurita I think she's right here, TypeConversionDispatcher is not a declarative class, it contains implementation details that I'm not sure we want to expose as public api. I think that enabling the user to convert new cornice types hiding the implementation details is the way to go.

You can argue that we can do it in an imperative way, something like

swagger = CorniceSwagger()
swagger.add_type_converter(MyType, MyTypeConverter)

and I'm keen on this.

+1 on refactoring the interface of the TypeConversionDispatcher, I don't like the __call__ either. To be completely honest, I also don't like that the the swagger document is produced on CorniceSwagger.__call__, I mean, I can't see a valid reason for the CorniceSwagger instance to be a callable.

@gabisurita
Copy link
Collaborator

I also +1 on removing the convert_xxx functions and rename __call__ for convert on the dispatchers. The CorniceSwagger class was meant to be something else than it became, so I'm also +1 to rename __call__ for generate or extract or something like that. But I'm also OK to leave this for a next PR.

@glasserc can you think of an use case where custom_type_converters and adefault_type_converter wouldn't be enough and you have to rewrite the TypeConversionDispatcher class? I think the approach in the last commit to take only these two parameters and keep a reference to a type dispatcher object is enough, I guess. If someone really wants to change the dispatcher this can be done by updating the reference on a CorniceSwagger object.

Copy link
Collaborator

@gabisurita gabisurita left a comment

Choose a reason for hiding this comment

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

Thank you. IMO it looks a lot better now, and it could probably be even better with the changes that @glasserc pointed out, but we can also do it in another moment.

Footnote: if this taking to long or you don't feel like doing it I can take from here and do the final changes.

"""

self.services = services

self.definitions = DefinitionHandler(ref=def_ref_depth)
typ_dispatcher = TypeConversionDispatcher(custom_type_converters, default_type_converter)
Copy link
Collaborator

@gabisurita gabisurita Jan 24, 2017

Choose a reason for hiding this comment

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

My feel is to do the same for the parameter dispatcher here, even if it doesn't take any additional parameters related to it on the __init__ call. Just to keep semantics.


self.definitions = DefinitionHandler(ref=def_ref_depth)
typ_dispatcher = TypeConversionDispatcher(custom_type_converters, default_type_converter)
self.definitions = DefinitionHandler(ref=def_ref_depth, typ_dispatcher=typ_dispatcher)
self.parameters = ParameterHandler(self.definitions,
ref=param_ref)
self.responses = ResponseHandler(self.definitions,
Copy link
Collaborator

@gabisurita gabisurita Jan 24, 2017

Choose a reason for hiding this comment

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

The response handler might also use custom type dispatchers.

self.typ_dispatcher = typ_dispatcher

def convert_schema(self, schema_node):
return convert_schema(schema_node, self.typ_dispatcher)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just call the type dispatcher here, we don't need to go through convert_schema.

def convert_schema(schema_node):

dispatcher = TypeConversionDispatcher()
def convert_schema(schema_node, dispatcher=TypeConversionDispatcher()):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now we have references to the dispatchers in the CorniceSwagger class, I would just remove these two and call the dispatchers directly from there.

@glasserc
Copy link
Contributor

Ah, sorry for describing an approach that you had already tried 😞. I guess I was late to the party. I tried to think of cases where adding specific converters for specific classes wouldn't be sufficient and I couldn't really think of any. The main case I thought of would be subclassing a colander schema type, for example subclassing colander.Float to define colander.Money. But that would clearly be a new type, we wouldn't want to change the existing conversions. And if we want to change how we convert colander.Float, we can do that using the custom_type_converters argument, so I guess that's fine. I guess I agree with your argument that TypeDispatcher is an implementation detail and not part of the interface. Yes, I prefer having fewer constructor arguments and more methods, I like that better too.

@smarzola
Copy link
Contributor Author

@gabisurita unfortunately I'm not able to dedicate time to this till next weekend. If you don't feel the urge to fix it, I'll be happy to do it!

@gabisurita
Copy link
Collaborator

@gabisurita unfortunately I'm not able to dedicate time to this till next weekend. If you don't feel the urge to fix it, I'll be happy to do it!

Thank you! We can wait, I guess. :)

Also, I don't have a strong opinion about adding converters imperatively. I guess both would be fine.

@gabisurita
Copy link
Collaborator

Any progress on this?

gabisurita added a commit that referenced this pull request Feb 4, 2017
* Declare ALL previously implemented options as CorniceSwagger
class attributes. The motivation for this is to simplify the
generator calls signatures and encourage applications to
implement their documentation by extending the this generator.

Ref #48
gabisurita added a commit that referenced this pull request Feb 13, 2017
This introduces a way to set custom type converters straight from
the CorniceSwagger class. It supports both a default type or a
mapping of custom types. This allows setting additional type
converters without having to patch the TypeConversionDispatcher
class.

Also depracates the `converters.convert_xxx` functions.

Replaces: #48
Related to: #46
@gabisurita
Copy link
Collaborator

Closed in favor of #65.

@gabisurita gabisurita closed this Feb 14, 2017
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.

4 participants