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

did validate_spec swagger schema agnostic #62

Closed
wants to merge 6 commits into from

Conversation

dutradda
Copy link

I have my own Swagger Schema Extension (with oneOf and anyOf support) and I need this changes to use my schema to validate under yours API.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.75% when pulling affe348 on dutradda:patch-1 into 634aadc on Yelp:master.

@coveralls
Copy link

coveralls commented Jan 14, 2017

Coverage Status

Coverage remained the same at 93.75% when pulling c9ec4f1 on dutradda:patch-1 into 634aadc on Yelp:master.

@dutradda
Copy link
Author

dutradda commented Feb 7, 2017

@analogue @jnb @prat0318

Can someone validate and merge my PR? This feature can be useful for other people too.

@dutradda
Copy link
Author

@gnufied @rockdog @bobtfish
hello!

Can someone give your opinion about this PR? Would be great if this feature were in the next version

@@ -57,7 +57,9 @@ def validate_spec_url(spec_url):
return validate_spec(load_json(spec_url), spec_url)


def validate_spec(spec_dict, spec_url='', http_handlers=None):
def validate_spec(spec_dict, spec_url='', http_handlers=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes more sense to provide a higher-level abstraction? Several possibilities come to mind, e.g. either passing in an arbitrary filesystem path, a file object from which we read or just the schema as JSON string. I wouldn't expose the internal python package + relative_path way of loading it as our API.

@coveralls
Copy link

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.02%) to 94.311% when pulling ef724d2 on dutradda:patch-1 into 18d524f on Yelp:master.

Copy link
Contributor

@sjaensch sjaensch left a comment

Choose a reason for hiding this comment

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

I think this makes much more sense, please fix the small docstring issues and we can merge this in!

@@ -69,6 +70,8 @@ def validate_spec(spec_dict, spec_url='', http_handlers=None):
http client built into jsonschema's RefResolver is used. This
is a mapping from uri scheme to a callable that takes a
uri.
:param schema_pkg: package of the json schema file.
:param schema_path: package relative path of the json schema file.
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't exist anymore. Please instead add a line documenting what schema_dict is supposed to be.

"""Validate a json document against a json schema.

:param spec_dict: json document in the form of a list or dict.
:param schema_pkg: package of the json schema file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obsolete as well.

@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage increased (+0.02%) to 94.311% when pulling 2055f7a on dutradda:patch-1 into 18d524f on Yelp:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 94.311% when pulling 2055f7a on dutradda:patch-1 into 18d524f on Yelp:master.

@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage increased (+0.05%) to 98.214% when pulling 1a48878 on dutradda:patch-1 into aea6e8c on Yelp:master.

@dutradda
Copy link
Author

dutradda commented Jan 29, 2018

@sjaensch Can you take a look in my PR? I had rebased it with the base branch.

@dutradda dutradda force-pushed the patch-1 branch 2 times, most recently from 19c7d4f to 6f13288 Compare February 6, 2018 22:10
 - did 'validate_apis' and 'validate_definitions' overwritable on 'validate_spec' function
 - renamed the parameter 'http_handlers' to 'custom_handlers' (it can be used for non-http handlers as well) and added a deprecation message on docs
 - added the 'apis_getter' and 'definitions_getter' parameters to 'validate_spec' function. These parameters can be used to customize how the apis and definitions objects are got
@dutradda
Copy link
Author

dutradda commented Feb 6, 2018

@sjaensch Can you take a look on my PR?

Copy link
Contributor

@sjaensch sjaensch left a comment

Choose a reason for hiding this comment

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

While this does seem to broaden the scope of swagger_spec_validator quite a bit, the additions seem constrained enough that I'm not too worried about it. But you didn't add any tests for the new API, are you sure you want me to merge it in like this? People might break your API, and we won't know / can't prevent it.

@dutradda
Copy link
Author

dutradda commented Feb 7, 2018

@sjaensch I tried to not broke the old api and tried to do it compatible. I just moved some functions to the args, so I can override this functions with another one. The functionally remains the same, because of this no extra tests are needed. I will check again for api breaks, but I think it is ok to merge.
I am sorry for my bad english.

@dutradda
Copy link
Author

dutradda commented Feb 7, 2018

I thought about it and I understood your points better.
So I will change my PR for just a little things, and I will create a new package with the functions I need to use and I will import yours functions. Thank you!

@dutradda dutradda closed this Feb 26, 2018
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.

None yet

3 participants