Skip to content

Conversation

@JKSenthil
Copy link
Contributor

County information is retrieved from CSBS (https://www.csbs.org/information-covid-19-coronavirus). Endpoint is /v2/locations?source=csbs. Added unit tests and added documentation in readme for the county addition.

Copy link
Owner

@ExpDev07 ExpDev07 left a comment

Choose a reason for hiding this comment

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

Damn, very well done! However, not sure I agree on the way the service is injected. I believe we'd be better off using a middleware to dynamically retrieve the data-source service.

E.g:

@api.before_request
def datasource():
    """
    Attaches the datasource to the request.
    """
    
    # Retrieve the datasource.
    source = request.args.get('source', type=str, default='jhu')

    # Attach source to request and return it.
    request.source = services.locations.get_datasource(source)
    pass

I am currently writing this right now.

@JKSenthil
Copy link
Contributor Author

Yeah, having a middleware would definitely be better. Feel free to let me know if you have any questions or other comments!

@ExpDev07
Copy link
Owner

I started some ground-work here: #115.

@ExpDev07
Copy link
Owner

ExpDev07 commented Mar 21, 2020

Hello, @JKSenthil! It should now be done, if you still want to add your source, a couple of things:

You're only allowed to return instances of Location. You may create a subclass like below (suggested implementation). You can add fields, but not edit nor remove them. If no data is available for it, just pass an empty string or 0:

from . import Location

class CbsLocation(Location):
    """
    A CBS location.
    """

    def __init__(self, id, country, state, county, coordinates, last_updated, confirmed, deaths):
        super().__init__(
            # General info.
            id, 'US', state, coordinates, last_updated,

            # Statistics.
            confirmed=confirmed,
            deaths=deaths,
            recovered=0, # not provided
        )

        # State and county.
        self.county = county
        self.state = state

    def serialize(self):
        """
        Serializes the location into a dict.


        :returns: The serialized location.
        :rtype: dict
        """
        serialized = super().serialize()

        # Update with new fields.
        serialized.update({
            'state'   : self.state,
            'county': self.county,
        })

        # Return the serialized location.
        return serialized

Then simply create your service for retrieving the locations, and register it in data/__init__.py. Your service should return a list of locations, not a dict like in your pull request. Simply just use index as id. Now your work is done. You don't have to do anything in the routes. You'll have fields such as "province" and "state" saying the same, but that's not really a big deal, and will provide compatibility across the data-sources.

I can work on implementing a more universal form for sending query params through to the services so we don't mess up the routes later. I just did it the easy way with country_code, but it should really be moved out of the routes module.

Happy coding! I might write a guide on adding data-sources later, but this ^ is a quick summary of it.

@ExpDev07 ExpDev07 linked an issue Mar 21, 2020 that may be closed by this pull request
@JKSenthil
Copy link
Contributor Author

Great, thanks for the details! I'll get to work.

Copy link
Owner

@ExpDev07 ExpDev07 left a comment

Choose a reason for hiding this comment

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

Well done :). I left a comment regarding the Last Update. To not confuse people, we should follow one format, which is UTC ISO (with an added 'Z' at the end for UTC).

confirmed = int(item['Confirmed'] or 0)
death = int(item['Death'] or 0)
coordinates = Coordinates(float(item['Latitude']), float(item['Longitude']))
last_update = item['Last Update']
Copy link
Owner

Choose a reason for hiding this comment

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

Should be parsed as ISO with an added 'Z' at the end. Use import datetime from datetime to achieve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@JKSenthil
Copy link
Contributor Author

I have finished up the csbs service and integrated it via the middleware, so everything is working. I think the only "issue" is that is /v2/locations.py, the serialising step passes in a timelines which the csbs doesn't use. For now, I pass in a dummy timelines in serialize(), is that fine?

@ExpDev07
Copy link
Owner

@JKSenthil

the serialising step passes in a timelines which the csbs doesn't use. For now, I pass in a dummy timelines in serialize(), is that fine?

Yes, that's fine. I'll fix it later. It's really just about checking if the Location is an instanceof TimelinedLocation before we pass in the timelines argument in the routes, I'll sort it, don't worry, for now it's fine.

Copy link
Owner

@ExpDev07 ExpDev07 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!

@ExpDev07
Copy link
Owner

Hey @all-contributors please add @JKSenthil for code, documentation, and tests.

@allcontributors
Copy link
Contributor

@ExpDev07

I've put up a pull request to add @JKSenthil! 🎉

@ExpDev07 ExpDev07 merged commit bb0f92d into ExpDev07:master Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add county data source for (US only)

2 participants