Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] added push_datapackage and pull_datapackage #66

Merged
merged 13 commits into from
Mar 31, 2016

Conversation

roll
Copy link
Member

@roll roll commented Mar 27, 2016

Work in progress will wait a discussion and https://github.com/frictionlessdata/testsuite-py tests added and passed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.0%) to 86.754% when pulling 63cce7a on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.6%) to 87.116% when pulling 1d8e7e5 on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.7%) to 87.007% when pulling 104b3ac on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.1%) to 86.628% when pulling 63c152a on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.5%) to 86.252% when pulling 8889a3d on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.5%) to 86.252% when pulling 50d19e3 on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-8.7%) to 90.029% when pulling 6fadf35 on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 98.266% when pulling 004235d on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.988% when pulling 2516f10 on feature/push-pull-datapackage into cf80599 on master.

@roll
Copy link
Member Author

roll commented Mar 29, 2016

@pwalsh @vitorbaptista
Please take a look.

It works with 9 datasets (push/pull to SQL/BigQuery) from testsuite-py:
https://travis-ci.org/frictionlessdata/testsuite-py/jobs/119227783

@pwalsh
Copy link
Member

pwalsh commented Mar 29, 2016

@roll 🎱

@pwalsh
Copy link
Member

pwalsh commented Mar 29, 2016

@roll also, really cool using behave for the integration tests!

@pwalsh
Copy link
Member

pwalsh commented Mar 31, 2016

@roll please get final signoff on this from @vitorbaptista

# Push
push_datapackage(
descriptor='descriptor_path',
backend='mystorage, '**<mystorage_options>)
Copy link
Contributor

Choose a reason for hiding this comment

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

The quote is in the wrong place

@vitorbaptista
Copy link
Contributor

@roll The code looks good, it just needs a few tweaks to be more consistent with the overall style of this package. For instance, instead of having pull_datapackage and push_datapackage, we'd have DataPackage.pull() as a class method which returns a DataPackage instance, and datapackage.push() as an instance method.

To be honest, I prefer DataPackage.from_storage() and datapackage.to_storage(), but that might be because "push" is a tricky word for me. It sounds the same as the Portuguese word "puxe", which means "pull". It takes me a second to translate, which makes pulling doors with "push" signs in them 😅

Push/pull are very common for devs, though, so I'm happy to keep it.

@roll
Copy link
Member Author

roll commented Mar 31, 2016

@vitorbaptista
It's following of our prev discussion - frictionlessdata/tableschema-py#51 (comment)

I think we've decided for now to have temp but working tooling:

- push_resource (filesystem -> db)
- pull_resource (db -> filesystem)
- push_datapackage (filesystem -> db)
- pull_datapackage (db -> filesystem)

So I've intentionally done it without touching DataPackage API and OOP-approach. I think we could integrate it with the main API after jsontableschm.Resource will be designed (we need to see how it will be looking) and then deprecate API from pushpull.py.

cc @pwalsh

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.988% when pulling 27a12c5 on feature/push-pull-datapackage into cf80599 on master.

virtualenv .python
source .python/bin/activate
pip install --upgrade -e .
pip install tox
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a common pattern? This feels like it should belong in the README instead, maybe in a "Running the tests" subsection inside "Developer notes".

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh my bad system .gitignore was broken

@vitorbaptista
Copy link
Contributor

You're right. Just check a few typos on the README (like exmples) and feel free to merge 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.988% when pulling b17a2ea on feature/push-pull-datapackage into cf80599 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 98.988% when pulling 5f198e9 on feature/push-pull-datapackage into cf80599 on master.

@roll
Copy link
Member Author

roll commented Mar 31, 2016

Great!

  • typos fixed
  • tested against testsuite-py again
  • merged

@roll roll merged commit 8bfdb07 into master Mar 31, 2016
@vitorbaptista vitorbaptista deleted the feature/push-pull-datapackage branch March 31, 2016 13:21
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.

None yet

4 participants