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

Makes attribute_map optional for serializing #60

Merged
merged 2 commits into from
Jan 31, 2019

Conversation

milancermak
Copy link
Contributor

@milancermak milancermak commented Jan 28, 2019

Description

Instead of having duplicated keys between attribute_map and deserialized_types, this PR allows to completely leave out attribute_map in serialized classes, if you don't want to do any renaming between the native and serialized representations.

An example:

class User:
    deserialized_types = {
        'name': 'str',
        'age': 'int'
    }

    def __init__(self, name='', age=0):
        self.name = name
        self.age = age

u = User('joe', 42)
serializer = DefaultSerializer()
serializer.serialize(u) # {'name': 'joe', 'age': 42}

Motivation and Context

This PR is a follow-up on the discussion in #58. I'm using the serializer in my projects too, as I found it super useful. However I found the repetition of attribute names unnecessary and decreasing the readability of my models.

Testing

I ran the tests locally (OS X, py3.6), all passed. Note I didn't add new tests to cover this functionality as it is only in discussion/evaluation phase. I'm happy to add some if you find the change acceptable.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nikhilym nikhilym added the feature-request:sdk SDK specific Feature Request label Jan 28, 2019
@nikhilym
Copy link
Contributor

Thanks @milancermak . We will review the code and get back to you soon.

Copy link
Contributor

@nikhilym nikhilym left a comment

Choose a reason for hiding this comment

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

These changes seems to be good @milancermak . Can we add some test cases to these changes and also update the docstrings for the serialize and deserialize methods, to reflect the functionality change?

Once the PR gets pushed, we can update the attribute management doc section as well with a note that users can use the DefaultSerializer class for their serialization/deserialization provided they have the deserialized_types map

@milancermak
Copy link
Contributor Author

Thanks @nikhilym. I did so in the last commit, can you have a look and tell me if there's anything missing? There was not that much docstrings to update.

@nikhilym nikhilym merged commit 7317d62 into alexa:master Jan 31, 2019
@nikhilym
Copy link
Contributor

Hey @milancermak. I have updated the docstrings and pushed in your changes to the repo. Thanks again for sending in the PR. I will update the PR once a new SDK is released with the changes on PyPI.

@nikhilym
Copy link
Contributor

nikhilym commented Feb 4, 2019

Release 1.6.0 contains the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request:sdk SDK specific Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants