-
Notifications
You must be signed in to change notification settings - Fork 306
Allow class creation via kwargs #4
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
Conversation
Signed-off-by: Garth Bushell <garth@garthy.com>
numberoverzero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request! This is a great usability improvement, and matches Python's typical constructor-based property initialization.
The only blocking request is moving to class-based decoration; the rest are stylistic.
I'll start on the CHANGELOG and update docs/examples to take advantage of the new syntax. Nice work!
| # PY2 Compatibility | ||
| __nonzero__ = __bool__ | ||
|
|
||
| def initkwargs(fn): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice solution!
I think we can expand the utility of this pattern a bit by moving the decorator to the class instead of the __init__ method. Then, we'll have a single place to inject additional post-processing in the future.
@util.apply_enhancements # or some name to indicate generality
class SomeModel(object):
def __init__(...)The PEP for class decorators suggests this is 3.0+ only, but things work fine here with 2.7.12.
Testing also gets easier when the individual enhancements can be defined on their own, with the actual decorator simply applying each function. Since the decorator returns the original class, other decorators can be stacked above this one:
# model.py
@user_defined_decorator
@util.extend
class SomeModel(object):
...
# util.py
def kwarg_init(self, **kwargs):
# TODO inspect via swagger_types
pass
def extend_model(cls):
cls.__init__ = kwargs_init
return cls| def init(self, **kwargs): | ||
| fn(self) | ||
| for kw, value in kwargs.items(): | ||
| if kw not in self.swagger_types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small nitpick, but swapping the iteration to loop over swagger_types instead of kwargs.items() is slightly faster and provides a more thorough error message.
Right now this will only display the first unknown attribute, instead of all the unknown attributes. By iterating the swagger_types and popping known keys, kwargs will only be non-empty if there are unknown attributes. This could eventually include required/optional logic and fail fast on missing arguments.
Using a Sentinel to skip available but not provided swagger types:
missing = Sentinel("Missing")
for attr in self.swagger_types:
value = kwargs.pop(attr, missing)
if value is not missing:
setattr(self, attr, value)
if kwargs:
raise(...)For 2.7/3+ compatibility, you'll probably want six.iterkeys in the raise.
| fn(self) | ||
| for kw, value in kwargs.items(): | ||
| if kw not in self.swagger_types: | ||
| raise TypeError("%s doesn't have atribute %s" %(self.__class__, kw)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo in "atribute" and please use str.format().
There may some lingering usage of the traditional style, but those will eventually be replaced.
|
|
||
| from .attach_volume_details import AttachVolumeDetails | ||
| from ...util import formatted_flat_dict | ||
| from ...util import formatted_flat_dict, initkwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to grab it in this review, but with the class decorator below we can move the use of formatted_flat_dict into the decorator as well; every model simply uses it for the same __repr__.
|
Sorry for the delay in looping back on this - as of v1.3.10 of the SDK (released yesterday, 27 Nov) classes can be initialized using kwargs. Documentation has also been updated to reflect this (e.g. https://oracle-cloud-infrastructure-python-sdk.readthedocs.io/en/latest/api/index.html#oci.object_storage.models.CreateBucketDetails) |
|
Just looping back again - going to close out this pull request as the changes to support providing keyword arguments when initialising a model object are now in the SDK |
Re-sync fork with oracle/oci-python-sdk master
Class creation is tedious
Using kwargs is much better
Signed-off-by: Garth Bushell garth@garthy.com