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

♻️ Formatting responses and abstracting out some of the models #30

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

dankolbman
Copy link
Contributor

Draws out some of the base models and establishes the common response formats.

TODO:
General test to test that all endpoints match this format

@dankolbman dankolbman self-assigned this Jan 17, 2018
@dankolbman dankolbman added the refactor Something needs to be done better label Jan 17, 2018
@dankolbman dankolbman changed the title ♻️ Formatting responses and abstracing out some of the models ♻️ Formatting responses and abstracing out some of the models Jan 17, 2018
parimalak
parimalak previously approved these changes Jan 17, 2018
Copy link
Contributor

@parimalak parimalak left a comment

Choose a reason for hiding this comment

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

looks good

'message': 'person deleted',
'content': {'persons': [person]}}, 200

return kf_response(person, 200, 'person deleted')

def _not_found(self, kf_id):
Copy link
Member

Choose a reason for hiding this comment

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

One test is failing: test_get_not_found in tests > person > test_resources.

Change this method to the following to fix:

    def _not_found(self, kf_id):
        """
        Temporary helper - will do error handling better later
        """
        return kf_response(code=404,
                           message='person with kf_id \'{}\' not found'.
                           format(kf_id))

Since the kf_response method already returns the tuple: (result dict, status code), we can just return the result of kf_response.

from dataservice.api.person.serializers import (person_model,
response_model)
from dataservice.api.person.serializers import (
person_model,
Copy link
Member

Choose a reason for hiding this comment

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

person_model looks like its no longer used in this file

Copy link
Member

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

Looks great, see file comments for requested changes

@dankolbman dankolbman changed the title ♻️ Formatting responses and abstracing out some of the models ♻️ Formatting responses and abstracting out some of the models Jan 17, 2018
@dankolbman dankolbman force-pushed the model-formatting branch 2 times, most recently from 0e5d0f4 to 110754d Compare January 17, 2018 20:47
znatty22
znatty22 previously approved these changes Jan 18, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@dankolbman dankolbman merged commit b59dfeb into master Jan 18, 2018
@dankolbman dankolbman deleted the model-formatting branch January 18, 2018 02:58
@dankolbman dankolbman mentioned this pull request Apr 2, 2018
alubneuski pushed a commit that referenced this pull request Oct 11, 2024
♻️ Formatting responses and abstracting out some of the models
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Something needs to be done better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants