Rebased on goodtables.next codebase

[roll]

Overview

Codebase update.

Issues

• fixes General pipeline refactoring #36
• fixes Add support for tabular data packages #66
• fixes Update management tools #75
• fixes Be specific about too many or too few columns in a row #91
• fixes Header column is empty should tell me which column #92
• fixes Schema Processor reports structural errors #93
• fixes Support validating JSON input data as well as XLS and CSV #95
• fixes Deep refactoring #106
• fixes Absolute row limit maximum in Pipeline #107
• fixes Update schema if extra/unordered headers has happened #117

[roll]

@pwalsh
Please take a look it's ready for a review - for related issues I've added examples of goodtables.next output or design notes.

Code here and readme - https://github.com/frictionlessdata/goodtables-py/tree/next-initial (PR interface hard to use in this case)

cc @amercader @georgiana-b

[pwalsh]

@roll any way to run tests of this code against a lot of those test files I had?

[roll]

@pwalsh
I've created issue for it - https://github.com/frictionlessdata/goodtables-py/issues/127 - will be adding it next

[pwalsh]

• missing desc. https://github.com/frictionlessdata/goodtables-py/blob/next-initial/setup.py#L51
• do we really want a CKAN profile? is this logic enough to get tabular data out of a CKAN instance (i guess not, and I think there are lots of custom things to consider)? https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/profiles/ckan.py#L15
• On the above, see what @georgiana-b does for CKAN here. On reflection, I really like adding this as a profile, but we probably need some way to automagically pull out all formats that tabulator supports (eg: when we add ods support to tabulator, the goodtables profile for ckan should be able to pull ods files). Finally, I think that @amercader should weight in here and decide if we can do this consistently enough to keep it as part of goodtables, or if we should just ship an example ckan profile in our examples directory
• I'm not a fan of passing in data (errors and table in this case), modifying it, and then not returning that modified data. https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/profiles/datapackage.py#L15

Based on the way profiles are used here, I don't see why the profile function needs errors and table passed in as arguments - they are always empty. Why not get rid of the empty list assignments and have

errors, tables = profile_handler(source, **options)  # sidenote: handler because more meaningful than func?

• Inspector.run instead of Inspector.inspect? https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/inspector.py#L59
• We def. want to keep this out of goodtables. you could submodule it in like we do with other static json files elsewhere, though https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/spec.json#L1
• On the above, I like the changes you made to data-quality-spec and they should be a PR there, as above, before this PR is merged. However, I don't think that report belongs here
• we are not setting an entry point for the goodtables cli. Why not? https://github.com/frictionlessdata/goodtables-py/blob/next-initial/setup.py#L41
• where are checks for things like file not found, http , io error, etc. coming from, if not here https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/checks/__init__.py#L1
• I've found that the Python ecosystem is not as conformant with semver as we'd like. So calling this 0.8 will definitely make some people upset. I think we should bump this to v1, and have this first release as rc1 https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/VERSION#L1
• Let's update our Python guidelines to use the new stuff that @vitorbaptista brought in here https://github.com/vitorbaptista/pyconuk_helloworld/blob/master/tox.ini - very very cool and useful.
• Can we update to cchardet in tabulator - much much faster https://github.com/frictionlessdata/tabulator-py/blob/master/setup.py#L26

[pwalsh]

@roll I'm done with my review, as above.

[roll]

@pwalsh
Thanks, pretty comprehensive review!

---

where are checks for things like file not found, http , io error, etc. coming from, if not here https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/checks/__init__.py#L1

I've tried to have check for every error but I've failed with this idea as not flexible enough. So for now source errors could be emitted:

• by profiles
• by framework itself - https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/inspector.py#L147

It gives much more flexibility now and in a future (e.g. when we'll move jts validation on table level - it will be not an exception handling but iterating over errors so check(exception) will not work). Also those table checks was just a boilerplate code.

---

I've added issues based on comments (as we've discussed to do first big chuck of work and then split additional work into small parts):

• profile naming and interface #124
• Improve test coverage #127
• Add builtin ckan preset to the library? #129
• Inspector method naming #130
• Move spec.json source to data-quality-spec repo #131
• Replace chardet by C version (cchardet) tabulator-py#104
• Integrate new features to python libraries packaging frictionlessdata.io#310

---

And I've pushed commits covering other comments.

[pwalsh]

@roll ok, but I do think that this is critical before merging this onto master, as without it, we've lost important functionality:

https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/inspector.py#L148

[roll]

@pwalsh
This one we're merging to next branch (check PR header). Then we will close other issues thru PRs to next branch. And then we will merge next to master (when it will be master ready, tested etc).

So I have no preference I could include https://github.com/frictionlessdata/goodtables-py/blob/next-initial/goodtables/inspector.py#L148 to this PR or close it as a next PR.

[pwalsh]

Ok. I don't know why the extra step, but go for it then, and I suggest to address these errors asap on the next branch.

[roll]

Ok. Thanks. Extra step because the code is not master ready but I want to make closing other issue reviwable by the team. To do not put everything in one gigantic PR (it's already pretty big though=).

PS.
But as we discussed with @amercader - starting from this merge he could be able already work with next branch while we're tweaking it.

[pwalsh]

Great. Let's go!