Skip to content

Added bill related data services#17

Merged
OriHoch merged 5 commits intohasadna:masterfrom
TomRaz:master
Jun 11, 2017
Merged

Added bill related data services#17
OriHoch merged 5 commits intohasadna:masterfrom
TomRaz:master

Conversation

@TomRaz
Copy link
Copy Markdown
Contributor

@TomRaz TomRaz commented Jun 9, 2017

No description provided.

@OriHoch OriHoch changed the title Added bill related data services [WIP] Added bill related data services Jun 9, 2017
@OriHoch
Copy link
Copy Markdown
Contributor

OriHoch commented Jun 9, 2017

thanks @TomRaz !

Quick review of the code looks amazing!

but tests are failing.. I guess it's still a work in progress?

I prefixed the PR title with [WIP] = work in progress - so we will know not to merge it yet

@OriHoch
Copy link
Copy Markdown
Contributor

OriHoch commented Jun 9, 2017

I'm not sure how useful the tests are in this case, when it's just a mapping of api to schema

up to you, as far as I'm concerned, I'm happy to merge without tests so you can go on to the datapackage

when we have the zip with csv files it will also be easy to review and test the data

@TomRaz
Copy link
Copy Markdown
Contributor Author

TomRaz commented Jun 10, 2017

@OriHoch - i agree about the tests, they are not really testing anything.
I added one file with integration tests that does a sanity check with each api, but added the "data_dependant_test" to it, so it won't run as part of the normal tests.

@OriHoch
Copy link
Copy Markdown
Contributor

OriHoch commented Jun 11, 2017

thanks @TomRaz let us know when it's ready for review/merge by removing the [WIP] prefix from title / and/or letting us know in a comment..
also, you can ask for review using the GitHub code review feature

@TomRaz TomRaz changed the title [WIP] Added bill related data services Added bill related data services Jun 11, 2017
@TomRaz
Copy link
Copy Markdown
Contributor Author

TomRaz commented Jun 11, 2017

@OriHoch - PR is ready for review.
I cannot change the "Reviewers" in github for some reason, maybe i don't have the permissions?

Copy link
Copy Markdown
Contributor

@OriHoch OriHoch left a comment

Choose a reason for hiding this comment

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

nice work, thanks!

@OriHoch OriHoch merged commit f86c3c8 into hasadna:master Jun 11, 2017
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.

2 participants