Skip to content

Conversation

@SeanCena
Copy link
Contributor

A very very minimal edit, but it would allow users to get aggregate data by country_code, province, etc. by adding a filter as a GET parameter. GET /api/v2/latest?country_code=US would solve #108 , for instance.

@ExpDev07
Copy link
Owner

I really like the way this was solved, nicely done. This should probably be documented in the README.md under the /latest endpoint :).

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

ExpDev07 commented Mar 21, 2020

Also, not a 100% sure if /latest should be responsible for the aggregation. Maybe it should instead be moved to /locations, and be put under “aggregated” or “latest” top-level? What do you think??

@gribok
Copy link
Contributor

gribok commented Mar 21, 2020

How is about testcases?

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

@gribok that - i do not know XD

@SeanCena
Copy link
Contributor Author

SeanCena commented Mar 21, 2020

Not sure about testcases. I'm not really sure what you mean by aggregated or latest top-level, would you mind explaining in more detail? (Sorry if these are really basic things I should know as a developer, I'm quite new to all this)

@ExpDev07
Copy link
Owner

ExpDev07 commented Mar 21, 2020

Like this:

/v2/locations?country_code=US

returns

{
  "latest": {
      "confirmed": 100000,
      "deaths": 4000,
      "recovered": 70000
  },
  "locations": [
  ]
}

"latest" being the sum of "latest" from all the locations returned (aggregated). Not sure whether we should call it latest or aggregated though.

All you do is basically move your code from latest() over to locations() in locations.py with some additions.

And it's okay aha @SeanCena

… filter locations by attributes other than just country_code
@ExpDev07
Copy link
Owner

ExpDev07 commented Mar 21, 2020

Very nicely done! Could you perhaps rename it to "latest" instead? I think overall it would make more sense given that's what it's called inside the locations, then I'll go ahead and merge.

@SeanCena
Copy link
Contributor Author

SeanCena commented Mar 21, 2020

Thanks, I moved the latest() code to locations(). For now, I put the aggregate data under "aggregate". Also, one of the side effects is that you can now pass any attribute (not just country_code) as an argument to /locations, which I just realized includes the double-underscore attributes of the timelinedlocations class. Might be a security issue, idk? Other than that, everything seems to work fine. make tests fails because there is no __main__ module apparently.

In response to your new comment, yes, I will rename it to latest.

SeanCena and others added 2 commits March 21, 2020 13:18
Also prevented user from filtering by double-underscore functions
@ExpDev07
Copy link
Owner

Thanks a lot! Looks good. Merging.

@ExpDev07 ExpDev07 merged commit e52874e into ExpDev07:master Mar 21, 2020
@ExpDev07
Copy link
Owner

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

@allcontributors
Copy link
Contributor

@ExpDev07

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

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.

See country total instead of province. Add United States aggregate stats

3 participants