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

Issue 621 Use Voluptuous for object marshaling + object documentation #786

Merged
merged 2 commits into from
Jul 14, 2015

Conversation

ihodes
Copy link
Member

@ihodes ihodes commented Jul 13, 2015

Use Voluptuous for object marshaling, and add object documentation
directly to those validation objects.

This will let us programmatically extract API documentation directly from
the code. Additionally, we get to use the same EDSL for validation and
marshaling objects.

Fixes #621

Migration:

alter table vcfs drop column validation_vcf;
alter table vcfs drop column f1score;
alter table vcfs drop column recall;
alter table vcfs drop column precision;

Review on Reviewable

@ihodes ihodes changed the title WIP issue 621 Issue 621 Use Voluptuous for object marshaling + object documentation Jul 13, 2015
@danvk
Copy link
Contributor

danvk commented Jul 13, 2015

Reviewed 5 of 11 files at r1.
Review status: 5 of 11 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


cycledash/init.py, line 26 [r1] (raw file):
codestatus_code


cycledash/init.py, line 28 [r1] (raw file):
Don't we want to send minified JSON over the wire?


cycledash/api/init.py, line 62 [r1] (raw file):
this link should include a SHA so that it doesn't rot


cycledash/api/init.py, line 71 [r1] (raw file):
move this out of the if/elif since it's identical in each case


cycledash/api/init.py, line 75 [r1] (raw file):
The docstring says it should be a dict, list or object.


cycledash/api/init.py, line 87 [r1] (raw file):
It's a little confusing that resp changes type here. Why not just use a different var?


cycledash/api/init.py, line 91 [r1] (raw file):
How about consolidating by doing concatenating to the returned tuple?

resp, status = resp[:2]
resp = marshal(resp, schema, envelope=envelope)
resp = camelcase_dict(resp)
headers = () if len(resp) == 2 else (resp[2],)
return (resp, status) + headers

cycledash/api/comments.py, line 62 [r1] (raw file):
Didn't like EpochField any more? Did it not work with Doc?


cycledash/api/tasks.py, line 18 [r1] (raw file):
Can you make the enum explicit in the Schema? (e.g. OneOf('a', 'b', 'c', ...))


cycledash/helpers.py, line 2 [r1] (raw file):
Don't think you need this


schema.sql, line 40 [r1] (raw file):
Is this related to the documentation change, or just cleanup?


Comments from the review on Reviewable.io

@ihodes
Copy link
Member Author

ihodes commented Jul 13, 2015

Review status: 2 of 12 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.


cycledash/api/init.py, line 91 [r1] (raw file):
Yeah this can definitely be tightened up—I tried something else :)


cycledash/api/comments.py, line 62 [r1] (raw file):
EpochField worked with flask-restful's fields—it's not part of voluptuous though, so this now works with voluptuous. Does the same thing, though.


cycledash/api/tasks.py, line 18 [r1] (raw file):
I'm all for that—I asked about that (adding an enum in the DB schema) in the PR that introduced Tasks, but IIRC we didn't want to go that route for some reason? So, I just went the route of following the database DB when creating this schema. Think I should file an issue to document the possible states?


cycledash/helpers.py, line 2 [r1] (raw file):
Good catch, surprised pylint didn't catch it.


schema.sql, line 40 [r1] (raw file):
Just cleanup—overdue.


Comments from the review on Reviewable.io

@danvk
Copy link
Contributor

danvk commented Jul 14, 2015

Looks good. The screenshot tests are failing on Travis. It's not clear to me why this PR would affect them, though.


Review status: 2 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


cycledash/api/init.py, line 41 [r2] (raw file):
nit: information about the specific types that an argument can take on makes more sense in the docs for that argument. The first line should just be a high-level description, like Filter data using a voluptuous Schema.


cycledash/api/init.py, line 42 [r2] (raw file):
Schema (sp)


cycledash/api/tasks.py, line 18 [r1] (raw file):
Sure, sounds good.


cycledash/helpers.py, line 2 [r1] (raw file):
It was the only addition to this file, so it was easy to spot. We run pylint with --errors-only, so it makes sense that we miss this sort of thing. Our JS linting for Cycledash is stricter.


Comments from the review on Reviewable.io

@ihodes
Copy link
Member Author

ihodes commented Jul 14, 2015

Thanks for the review!

For posterity, screenshots failing due to earlier change that no longer hid comment time; this is fixed in the following commit.


Review status: 2 of 12 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


cycledash/api/init.py, line 41 [r2] (raw file):
Definitely, good call. A worthwhile nit!


cycledash/api/tasks.py, line 18 [r1] (raw file):
Cool, filed: #788


Comments from the review on Reviewable.io

Use Voluptuous for object marshalling, and add object documentation
direcly to those validation objects.

This will let us programatically extract API documentation directly from
the code. Additionally, we get to use the same EDSL for validation and
marshalling objects.

Document marshalled fields in code for API modules

Fixes #621
Need to hide .time so the comment times don't cause screenshots to differ.
ihodes added a commit that referenced this pull request Jul 14, 2015
Issue 621 Use Voluptuous for object marshaling + object documentation
@ihodes ihodes merged commit 99431cf into master Jul 14, 2015
@ihodes ihodes deleted the issue-621- branch December 23, 2015 16:53
@hammer hammer unassigned danvk Feb 8, 2016
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