-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add new action to infer a tabular resource schema #82
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #82 +/- ##
==========================================
- Coverage 84.55% 83.82% -0.73%
==========================================
Files 24 24
Lines 2117 2139 +22
==========================================
+ Hits 1790 1793 +3
- Misses 327 346 +19
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far @aivuk, I added some comments
BTW this coverage messages are really annoying. I guess they'll go away when you add tests but I'd be happy to turn them off, if you find how to do it
@@ -6,6 +6,7 @@ | |||
|
|||
from werkzeug.datastructures import FileStorage as FlaskFileStorage | |||
import ckan.plugins as p | |||
import ckan.lib.uploader as uploader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
resource = t.get_action('resource_show')( | ||
{}, {u'id': data_dict['resource_id']}) | ||
|
||
source = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the resource['format'] is not one of ckanext.validation.formats
we should raise a ValidationError
with : "Not a valid format to infer the resource schema"
# TODO: check for valid formats | ||
fric_resource = Resource({'path': source, 'format': resource.get('format', 'csv').lower()}) | ||
fric_resource.infer() | ||
resource['schema'] = fric_resource.schema.to_json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get an error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can. an example, if you don't add the format, because the uploaded file don't have an extension, the frictionless throws a exception saying that it can't infer the schema because the file is not tabular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so let's catch these exceptions and wrap them in a raised ValidationError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Adrià Mercader <[email protected]>
…om:frictionlessdata/ckanext-validation into feature/76-add-resource-table-schema-infer
removed it for now |
Implement new action as described on #76.