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

Have models inherit from base class, support model polymorphism #128

Closed
wants to merge 5 commits into from
Closed

Have models inherit from base class, support model polymorphism #128

wants to merge 5 commits into from

Conversation

jlumpe
Copy link
Contributor

@jlumpe jlumpe commented Oct 14, 2016

Re-worked model generation to use a base Model class that defines most of the functionality. The current implementation subclasses object and adds lambdas with closures to the __dict__ in lieu of methods. This allows for custom extensions as well as I think being cleaner and more Pythonic.

One issue with dynamic generation of model classes is that the schema properties could be named anything so there will always be possible conflicts with the base model methods/attributes. I've attempted to minimize this by prefixing all class-level attribute names that are part of the public API with a single underscore. This is unusual but is similar to the collections.namedtuple class which also creates dynamic classes with arbitrary attribute names. Truly private attributes are prefixed with a double underscore.

Quick list of changes:

  • Property values stored in Model.__dict attribute instead of standard Model.__dict__ so that attribute access can be customized.
  • Attribute access prioritizes class attributes (methods, etc.) over model properties, so that the public API will never be accidentally hidden. This is only an issue with property names beginning with underscores.
  • Property values can be accessed explicitly using model['prop_name']. This works for setting and deleting too.
  • Deleting a property defined in the spec (through del model.property or del model['property']) will set it to None instead of deleting it entirely.
  • Models function as a container of property names for __iter__ and __contains__ (similar to a dictionary).
  • Model._additional_props is now dynamic.
  • Docstrings are still generated lazily.
  • marshal() and unmarshal() methods renamed _marshal() and _unmarshal().
  • _to_dict() gets dictionary of property values.
  • create_model_type() allows using a custom base class.
  • Sorry but I think I overwrote the newest fix allowing "self" properties. The Model._from_dict() method
    is pretty much the same thing as the new create().

Also, polymorphism is now implemented as defined as per the Swagger 2.0 spec. If a model spec has the discriminator property defined, this value will be checked when unmarshaling data to choose an alternative model.

TODO:

  • Write tests for new functionality
  • Instantiating with additional properties when schema['additionalProperties'] == False raises an exception, but no checking is done when setting attributes afterward.
  • I think Model.__dir__() should be more consistent with the standard implementation and return private and class attributes as well. To get the current property names you can just iterate over it. For now I've kept like it was before to pass the existing test.
  • Model.__repr__() can get very long, maybe truncate it?
  • Validate properties when they are set?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 95.648% when pulling 022353b on jlumpe:models into dac21a8 on Yelp:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 95.648% when pulling 022353b on jlumpe:models into dac21a8 on Yelp:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 95.648% when pulling 022353b on jlumpe:models into dac21a8 on Yelp:master.

@coveralls
Copy link

coveralls commented Oct 14, 2016

Coverage Status

Coverage decreased (-1.7%) to 95.644% when pulling 66c12cf on jlumpe:models into dac21a8 on Yelp:master.

@jlumpe
Copy link
Contributor Author

jlumpe commented Oct 17, 2016

I can write some additional tests for this, but it would be good to get feedback on what I've got first.

@coveralls
Copy link

coveralls commented Nov 28, 2016

Coverage Status

Coverage decreased (-1.6%) to 95.933% when pulling 1457740 on jlumpe:models into 2446114 on Yelp:master.

@coveralls
Copy link

coveralls commented Dec 15, 2016

Coverage Status

Coverage decreased (-2.5%) to 95.052% when pulling 516d61a on jlumpe:models into 2446114 on Yelp:master.

@coveralls
Copy link

coveralls commented Dec 15, 2016

Coverage Status

Coverage decreased (-2.5%) to 95.017% when pulling 09458d3 on jlumpe:models into 2446114 on Yelp:master.

@sjaensch
Copy link
Contributor

@jlumpe this is looking really good! Could you add a couple of tests for model polymorphism?

Also, do you consider all of your TODOs blockers for shipping this? Some of those seem like separate issues, if you agree please file them accordingly.

@jlumpe
Copy link
Contributor Author

jlumpe commented Dec 16, 2016

@sjaensch Thanks for reviewing this. I would say other than the tests everything in the TODO list is more of a "nice to have" and can be done in separate PRs later.

If this all looks good I can get started on writing the remaining tests right away. Just wanted to confirm that the API is acceptable, I know prefixing non-private methods with an underscore is unconventional but I don't see much of a way around it here.

Also, it looks like all the Travis checks for Python 2.6 keep failing due to the use of some 2.7-specific syntax. I'll have to clean that up as well.

@sjaensch
Copy link
Contributor

@jlumpe honestly I don't have a better solution that makes sure we won't have name clashes with model fields, so let's do it.

@coveralls
Copy link

coveralls commented Dec 19, 2016

Coverage Status

Coverage decreased (-2.5%) to 95.021% when pulling 0f31f61 on jlumpe:models into b192c19 on Yelp:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 95.021% when pulling 0f31f61 on jlumpe:models into b192c19 on Yelp:master.

@macisamuele
Copy link
Collaborator

@jlumpe I took your changes and added tests for discriminator feature in #159.
If you feel that something else is missing please let us to know.
And thanks a lot for your great contribution on this project ... it is really appreciated!

macisamuele added a commit that referenced this pull request Mar 10, 2017
Have models inherit from base class, support model polymorphism (continuation of  #128)
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