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

Catch ValueErrors as in KSP-CKAN/CKAN-meta#1035 #1648

Merged
merged 1 commit into from
Apr 9, 2016

Conversation

techman83
Copy link
Member

A small modification to 'ckan-validate.py' which is used in our testing. Gets rid of the big Traceback, we can probably do more with it.

@Dazpoet
Copy link
Member

Dazpoet commented Apr 9, 2016

After a bit of googling I think I've gotten myself a good enough understanding of this to merge it in good conciense, also it passes travis.

@Dazpoet Dazpoet merged commit e95f656 into KSP-CKAN:master Apr 9, 2016
@techman83 techman83 deleted the improveValidation branch April 10, 2016 02:43
@techman83
Copy link
Member Author

It's a pretty safe one, really only breaks Jenkins + Indexing and easy to revert in that case.

@Dazpoet
Copy link
Member

Dazpoet commented Apr 10, 2016

After looking a few more times, because who doens't love learning? I wonder if ValueError should've also been added to https://github.com/KSP-CKAN/CKAN/blob/master/bin/ckan-validate.py#L6 or have I, once again, totally misunderstood things?

@techman83
Copy link
Member Author

@Dazpoet hmm, Python is something I only dabble in. To my eyes you import from to access the method/function directly.

For example you can do this

import json
from jsonschema import validate

validate(json.load(ckan_file), schema)

versus

import json
import jsonschema

jsonschema.validate(json.load(ckan_file), schema)

I haven't fully grok'd pythons error handling, but I don't think those even need to be exported. It certainly catches ValueError without doing as such.

@Dazpoet
Copy link
Member

Dazpoet commented Apr 11, 2016

Thanks for expanding on the issue, I'm doing my best to learn :)

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