Skip to content
This repository has been archived by the owner on Sep 17, 2024. It is now read-only.

Expose data on api #28

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion victims/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from aiocache import cached, caches
from sanic import Sanic
from sanic.response import redirect
from sanic.response import json, redirect
from sanic_compress import Compress
from sanic_jinja2 import SanicJinja2

Expand Down Expand Up @@ -47,3 +47,10 @@ async def home(request):
@jinja.template("about.html")
async def about(request):
return {"title": TITLE, "url_path": "/about.html"}


@app.route("/data.json")
async def data(request):
cases = await get_cases()
cases = [case.to_JSON() for case in cases]
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no case.to_JSON() anymore, right?

Copy link
Author

Choose a reason for hiding this comment

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

you're right, my bad

return json(cases)
31 changes: 31 additions & 0 deletions victims/models.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import json

from dataclasses import dataclass
from datetime import date
from typing import List
Expand Down Expand Up @@ -27,6 +29,20 @@ def is_valid(self):

return True

def to_JSON(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

"""
Serialize object to json
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

d = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does d stands for? May we have a more meaningful variable name here?

"url": self.url,
"source": self.source,
"title": self.title,
"image_or_video": self.image_or_video,
"summary": self.summary,
"case_id": self.case_id,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any special reason to not use dict(self) and keep the descriptive verbose approach? IMHO this is technical debt.

Copy link
Author

@baltazarix baltazarix Oct 16, 2018

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

return json.dumps(d, sort_keys=True)


@dataclass
class Case:
Expand All @@ -48,3 +64,18 @@ def tags_and_colors(self):

def is_valid(self):
return bool(self.when)

def to_JSON(self):
"""
Serialize object to json
"""
d = {
"id": self.id,
"aggressor_side": self.aggressor_side,
"when": str(self.when),
"state": self.state,
"city": self.city,
"tags": self.tags,
"stories": [story.to_JSON() for story in self.stories],
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should not encode story as JSON since the whole items is encoded in the end.

Copy link
Author

Choose a reason for hiding this comment

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

good point, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@baltazarix do you mind adding a simple test for this endpoint? It's pretty simple and can avoid assure we're serializing data correctly.

Copy link
Author

Choose a reason for hiding this comment

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

@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 Case instance, 2 Story instances), then compare response content.

Feel free to tell me if I'm missing something :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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…

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any special reason to not use dict(self) and keep the descriptive verbose approach? IMHO this is technical debt.

return json.dumps(d, sort_keys=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just the same as my comments in the other dataclass ; )