Skip to content

Conversation

@ibhuiyan17
Copy link
Contributor

@ibhuiyan17 ibhuiyan17 commented Apr 4, 2020

PR for adding the New York Times timeseries as a source, mentioned in #224

todo

  • Add unit tests for NYT source
  • Working v2 endpoints
  • Generate mock data
  • Add regression tests
  • Update documentation

** removed "Working v1 endpoints" from todo since current implementation of v1 doesn't support different sources

@ibhuiyan17 ibhuiyan17 changed the title [WIP] Source nyt [WIP] Source NYT Apr 4, 2020
@Kilo59 Kilo59 linked an issue Apr 7, 2020 that may be closed by this pull request
@Kilo59 Kilo59 added enhancement New feature or request source: nyt New York Times labels Apr 7, 2020
Copy link
Collaborator

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

I realize it's still WIP.
The ci failure is due to some pylint warnings and formatting issues.
The formatting can be applied by running invoke fmt

Here are the pylint warnings.

************* Module app.services.location.nyt
app/services/location/nyt.py:36:0: C0116: Missing function or method docstring (missing-function-docstring)
app/services/location/nyt.py:36:23: W0613: Unused argument 'category' (unused-argument)
app/services/location/nyt.py:48:4: W0107: Unnecessary pass statement (unnecessary-pass)
app/services/location/nyt.py:54:-1: W0105: String statement has no effect (pointless-string-statement)
app/services/location/nyt.py:2:0: W0611: Unused import csv (unused-import)
app/services/location/nyt.py:3:0: W0611: Unused datetime imported from datetime (unused-import)
app/services/location/nyt.py:8:0: W0611: Unused httputils imported from utils (unused-import)
-----------------------------------
Your code has been rated at 9.77/10

@ibhuiyan17
Copy link
Contributor Author

Yep, still a work in progress, but thanks for pointing this out!

@ibhuiyan17 ibhuiyan17 marked this pull request as draft April 8, 2020 23:45
@nischalshankar
Copy link
Contributor

There were a few things we wanted to check in with you guys.

Output for NYT:
Currently, the NYT source doesn't provide coordinates, so we have just added null to those output values. NYT provides a fips number (specifies geographic location), which we could add to the output directly or potentially use it to translate to coordinates. Another option is we could use the (county, state) coordinates from csbs to find the coordinates of NYT locations, but this could be an inefficient solution. A way we could do this might be to store the specific coordinates in a file or a database.

Testing:
We tested all the v2 endpoints for NYT. The only content check we added is done for get locations which follows the same format as the other sources. Let us know if you want any additional content checks for any of the other sources. Also, we did not check for the specific value of country population since it could possibly change.

@ibhuiyan17 ibhuiyan17 marked this pull request as ready for review April 12, 2020 20:58
@ibhuiyan17 ibhuiyan17 requested a review from Kilo59 April 12, 2020 20:58
Copy link
Collaborator

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

@ibhuiyan17 @nischalshankar Looks good!
I think nulling the coordinates values is a good approach for the initial implementation.
No reason to hold back using this source just because of that.

Just have a couple of minor points that I commented on below.

@Kilo59 Kilo59 requested a review from ExpDev07 April 12, 2020 22:13
@ibhuiyan17 ibhuiyan17 changed the title [WIP] Source NYT Source NYT Apr 13, 2020
@ibhuiyan17 ibhuiyan17 requested a review from Kilo59 April 13, 2020 00:31
@ibhuiyan17
Copy link
Contributor Author

@Kilo59

Just finished resolving the points you commented on. Please let me know if there's anything else, thanks!

@Kilo59 Kilo59 merged commit 78da9fa into ExpDev07:master Apr 13, 2020
@Kilo59
Copy link
Collaborator

Kilo59 commented Apr 13, 2020

@all-contributors please add @nischalshankar for code, docs

@allcontributors
Copy link
Contributor

@Kilo59

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

@Kilo59
Copy link
Collaborator

Kilo59 commented Apr 13, 2020

@all-contributors please add @ibhuiyan17 for docs

@allcontributors
Copy link
Contributor

@Kilo59

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request source: nyt New York Times

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using timeseries provided by NYT

3 participants