Skip to content

Python3 support #11

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

Merged
merged 3 commits into from
Apr 15, 2015
Merged

Python3 support #11

merged 3 commits into from
Apr 15, 2015

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Feb 26, 2015

Related to #10

There are definition still some test flakes here, but they magically stopped happening locally so I decided to open this PR for now.

The error was:

swagger_spec_validator.common.SwaggerValidationError: Unresolvable JSON pointer: 'definitions/simpleTypes

which is strange because simpleTypes doesn't appear anywhere in this repo, but it does exist in jsonschema/schemas/draft4.json.

@dnephin dnephin force-pushed the python3 branch 2 times, most recently from 83a4152 to 1373f65 Compare February 27, 2015 07:44
@dnephin
Copy link
Contributor Author

dnephin commented Feb 27, 2015

I believe I've fixed the bug in jsonschema, I'm going to submit a PR

@dnephin
Copy link
Contributor Author

dnephin commented Feb 27, 2015

Opened python-jsonschema/jsonschema#201, let's see if we get a response. For now we could just mark these failing tests as "broken on py3" so they get skipped

resolver = RefResolver(
'file://{0}'.format(schema_path),
schema,
store={'http://swagger.io/v2/schema.json': schema})
Copy link
Contributor

Choose a reason for hiding this comment

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

How will the store mapping help? Also, the schema could either be 1.2 or a 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this should be store=schema['id']

I believe it should avoid a network call. The way refs are looked up, they use urljoin(current_scope, json_pointer) where json_pointer is some #/path/to/ ref. If anything uses the absolute path to the schema, it would cause a network call.

I guess this might not happen very often in practice. It might have been triggered by the bug that is being fixed in jsonschema under python 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the key id is not always present in the schema (it is present in 2.0 schema but not in api_declaration and resource_listing schema of 1.2).

Also, from signature of RefResolver, store takes a dict (id would be a str).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I meant store={schema['id']: schema}

This should already be covered by tests. I'll double check that it works with 1.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about this again api_declaration and resource_listing aren't valid jsonschema, which is why they don't have id. They're being used as the "json object" being validated against the swagger spec schema.

I can change this to schema.get('id') just to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just revert this part of it. It's not really important. We can revisit it if we ever hit this case (of things using absolute urls).

@prat0318
Copy link
Contributor

prat0318 commented Mar 2, 2015

lgtm. Ship it may be after the python-jsonschema/jsonschema#203 merge?

prat0318 added a commit to prat0318/pyramid_swagger that referenced this pull request Mar 3, 2015
swagger_spec_validator. This commit only covers validation of swagger
2.0 spec and in no way handles serving of 2.0 compatible endpoint.

It currently breaks py33 and py34 tests. This should be rectified with
the merge of Yelp/swagger_spec_validator#11.
prat0318 added a commit to prat0318/pyramid_swagger that referenced this pull request Mar 3, 2015
swagger_spec_validator. This commit only covers validation of swagger
2.0 spec and in no way handles serving of 2.0 compatible endpoint.

It currently breaks py33 and py34 tests. This should be rectified with
the merge of Yelp/swagger_spec_validator#11.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 84.0% when pulling 5a43e5d on dnephin:python3 into a4cfcd9 on Yelp:master.

@prat0318
Copy link
Contributor

A new release of jsonschema is still not available, can we add the commit sha for the time-being in this PR to make the tests pass.

@dnephin
Copy link
Contributor Author

dnephin commented Apr 15, 2015

Ya, we can try that. Right now we're just installing any unpinned version. I don't think install_requires supports git urls, so we'd have to pin a version for the test suite. I'll give that a shot.

We'll have to pin the same version in the pyramid_swagger testsuite if we want to use it there as well.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 84.0% when pulling 59e20e2 on dnephin:python3 into a4cfcd9 on Yelp:master.

@dnephin
Copy link
Contributor Author

dnephin commented Apr 15, 2015

Ok, tests pass with pinned version of jsonschema. Look good?

@prat0318
Copy link
Contributor

yeah lgtm.

prat0318 added a commit that referenced this pull request Apr 15, 2015
@prat0318 prat0318 merged commit 5a47ed7 into Yelp:master Apr 15, 2015
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.

3 participants