Skip to content

Conversation

@tjann
Copy link
Contributor

@tjann tjann commented Aug 24, 2020

No description provided.

@tjann tjann requested a review from shifucun August 24, 2020 09:20
@tjann tjann marked this pull request as ready for review August 24, 2020 09:20
@tjann tjann requested review from beets and pradh August 24, 2020 10:26
Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

I have some meta questions around how we're structuring this library, as well as whether we should do the selection to cover most geos in this library or in the mixer (my vote for the latter, so that we get standardized behavior across clients).

Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

thanks, agree with the meta-changes you outlined. some outstanding comments still in df_builder.py

@tjann tjann requested review from beets and shifucun August 25, 2020 05:54
@shifucun
Copy link
Contributor

The structure looks much cleaner now!

@tjann tjann requested a review from shifucun August 25, 2020 16:43
Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

thanks for the investigation, i think this solution works well!

i still have some unaddressed comments on df_builder.py (they seem to have gotten collapsed). could you take a look at those? mostly around readability and reusability of the selection logic.

tjann added 2 commits August 25, 2020 12:50
…lder; add test for raising no data error; fix various tests that were returning the type HTTPError instead of an instance of HTTPError.
Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

thanks for making all the changes!

@tjann tjann merged commit ffba5fd into datacommonsorg:master Aug 26, 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.

3 participants