Expose data on api #28
Conversation
victims/models.py
Outdated
| @@ -27,6 +29,20 @@ def is_valid(self): | |||
|
|
|||
| return True | |||
|
|
|||
| def to_JSON(self): | |||
There was a problem hiding this comment.
to_JSON is a syntax that does not follow PEP8:
Method Names and Instance Variables
Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.
Can we rename it to to_json?
victims/models.py
Outdated
| def to_JSON(self): | ||
| """ | ||
| Serialize object to json | ||
| """ |
There was a problem hiding this comment.
This short docstring could be in one:
def to_json(self):
"""Serialize object to json"""
But is this docstring really necessary? I feel like the name to_json says it all and the docstring is redundant. What do you think about it?
victims/models.py
Outdated
| """ | ||
| Serialize object to json | ||
| """ | ||
| d = { |
There was a problem hiding this comment.
What does d stands for? May we have a more meaningful variable name here?
victims/models.py
Outdated
| "tags": self.tags, | ||
| "stories": [story.to_JSON() for story in self.stories], | ||
| } | ||
| return json.dumps(d, sort_keys=True) |
There was a problem hiding this comment.
Just the same as my comments in the other dataclass ; )
- corrected method names according to PEP8 - removed redundant docstrings - more meaningful variable name inside 'to_json' method
|
@cuducos, Just pushed changes according to your comments. Thanks for time you've spent on this review and also for right remarks. |
| "image_or_video": self.image_or_video, | ||
| "summary": self.summary, | ||
| "case_id": self.case_id, | ||
| } |
There was a problem hiding this comment.
Any special reason to not use dict(self) and keep the descriptive verbose approach? IMHO this is technical debt.
There was a problem hiding this comment.
I used variable here cause I had to somehow serialize datetime.date. After my research on network I can propose to implement custom json encoder class, which will handle datetime.date value. With usage of dataclasses.asdict whole to_json method may look like this:
def to_json(self):
return json.dumps(asdict(self), cls=DateEncoder, sort_keys=True)There was a problem hiding this comment.
This is a good way. Alternatively simpler though:
def to_json(self):
data = asdict(self)
data['when'] = date['when'].strftime("%Y-%m-%d")
return json.dumps(data, sort_keys=True)
| "city": self.city, | ||
| "tags": self.tags, | ||
| "stories": [story.to_JSON() for story in self.stories], | ||
| } |
There was a problem hiding this comment.
Any special reason to not use dict(self) and keep the descriptive verbose approach? IMHO this is technical debt.
| @app.route("/data.json") | ||
| async def data(request): | ||
| cases = await get_cases() | ||
| cases = [case.to_JSON() for case in cases] |
There was a problem hiding this comment.
There's no case.to_JSON() anymore, right?
I think you missed some points. Could you review or come back with questions to help me clarify my points? So sorry if I was unable to make myself clearer : ( |
| "state": self.state, | ||
| "city": self.city, | ||
| "tags": self.tags, | ||
| "stories": [story.to_JSON() for story in self.stories], |
There was a problem hiding this comment.
This line should not encode story as JSON since the whole items is encoded in the end.
There was a problem hiding this comment.
@baltazarix do you mind adding a simple test for this endpoint? It's pretty simple and can avoid assure we're serializing data correctly.
There was a problem hiding this comment.
@turicas, sure! I was thinking about it but I've had no idea how test scenario should look like. Only ideas I have are:
- checking for status code
- create some test data (e.g. 1
Caseinstance, 2Storyinstances), then compare response content.
Feel free to tell me if I'm missing something :)
There was a problem hiding this comment.
Feel free to tell me if I'm missing something
Take a look at existing test_home_contents: we test some contents that we expect to see in the response. Name of the stories, maybe dates formatted as str (as they are the bottleneck of this serialization) etc…
There was a problem hiding this comment.
Take a look at existing
test_home_contents: we test some contents that we expect to see in the response. Name of the stories, maybe dates formatted as str (as they are the bottleneck of this serialization) etc…
Yep, I wanted to make something similar, sorry if I wasn't clear enough.
expose data from models
Case,Storyat/data.jsoncloses #21