socialfoundations / folktables

Datasets derived from US census data
MIT License
234 stars 20 forks source link

Created utilities to (down)load datasets and added custom exceptions. #29

Closed RicardoJSandoval closed 2 years ago

RicardoJSandoval commented 2 years ago

Hi! In this pull request I'm adding the following files:

This is the first pull request from the series of pull requests I specified here.

mrtzh commented 2 years ago

LGTM - would you be able to add some tests for the file path and download logic? Thanks!

RicardoJSandoval commented 2 years ago

LGTM - would you be able to add some tests for the file path and download logic? Thanks!

Just pushed a set of tests! Note in order to be able to make mock requests in the tests, I'm using the requests-mock package (linked here). Therefore, we'll have to update the requirements.txt file. I'll go ahead add another pull request for this.

I also added a new exception (InvalidFilePath) and improved the load_utils.extract_content_from_zip function.

ludwigschmidt commented 2 years ago

Looks good! I really like that we're using pathlib everywhere. I left a few comments. Also please make sure the tests pass before we merge the PR. The requests-mock dependency may have to go into tests_require of setup.py instead of requirements.txt.

RicardoJSandoval commented 2 years ago

Hi Ludwig! Thanks for all the comments/observations. I'll go ahead and answer each of them individually. I'll also make sure to add the requests-mock dependency to test_require in setup.py and remove it from requirements.txt.

ludwigschmidt commented 2 years ago

Great thank you! Let me know when I should re-run the tests in case you can't do that yet (I think there may be some restrictions on first-time committers). I just re-ran them and they didn't succeed yet.

RicardoJSandoval commented 2 years ago

I'll push some new code. It seems like the requests-mock package wasn't installed before running the integration tests, so I just added a line to the GitHub workflow to, hopefully, solve this issue.

mrtzh commented 2 years ago

Thanks for all the work! Merging now.