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

Schema fixes #318

Merged
merged 4 commits into from Dec 3, 2018
Merged

Schema fixes #318

merged 4 commits into from Dec 3, 2018

Conversation

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Dec 3, 2018

Some tweaks to #311.

@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Dec 3, 2018

@tiangolo please let me know if this is alright. Seemed simpler to implement my thoughts than put them all in comments. Look at the two separate commits as they separate the work into rearrangement and changes.

@codecov
Copy link

@codecov codecov bot commented Dec 3, 2018

Codecov Report

Merging #318 into json-schema-extra will not change coverage.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           json-schema-extra   #318   +/-   ##
================================================
  Coverage                100%   100%           
================================================
  Files                     14     13    -1     
  Lines                   1770   1761    -9     
  Branches                 338    340    +2     
================================================
- Hits                    1770   1761    -9

Copy link
Collaborator

@tiangolo tiangolo left a comment

Cool! I like it.

Seemed simpler to implement my thoughts than put them all in comments.

Absolutely, thanks for putting in the time to do it.

@@ -141,12 +87,14 @@ def __init__(
self.allow_none: bool = allow_none
self.parse_json: bool = False
self.shape: Shape = Shape.SINGLETON
self._schema: Schema = schema
self._schema: 'schema.Schema' = schema
Copy link
Collaborator

@tiangolo tiangolo Dec 3, 2018

Great. I thought it was necessary to import schema for this to work.

tests/test_schema.py Outdated Show resolved Hide resolved
@tiangolo
Copy link
Collaborator

@tiangolo tiangolo commented Dec 3, 2018

After this, I would like to submit another PR to change ._schema to .schema, as I've found myself using the schema attribute all the time while building stuff based on Pydantic.

But I'll wait for this PR to be finished to avoid conflicts. ...Or let me know if you would prefer me to create a PR to this branch before merging.

@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Dec 3, 2018

fine by me, I've renamed _schema to schema

@samuelcolvin samuelcolvin merged commit 17daf9b into json-schema-extra Dec 3, 2018
9 checks passed
@samuelcolvin samuelcolvin deleted the schema-fixes branch Dec 3, 2018
samuelcolvin added a commit that referenced this issue Dec 3, 2018
* Add additional parameters to Schema for validation and annotation (#311)

* Add tests for validation declared via Schema class in defaults

* Add validations to field from declarations in Schema

* Add annotations in generated JSON Schema from validations via Schema

* Augment tests for schema generation

* Simplify validations from Schema in fields, from coverage hints

* Update schema test to use the spec plural "examples"

* Add docs and simple example of the additional parameters for Schema

* Update history

* Fix number of PR in HISTORY

* Refactor check for numeric types, remove break to make coverage happy

* Fix typo in docs, I confused gt with maximum

* Finish docstring for Schema (I had forgotten about it)

* Implement code review requests and lenient_issubclass with tests

* Move Schema to its now file to extract from fields.py but avoid circular imports

* Control coverage

* Schema fixes (#318)

* rearrange code

* cleanup get_annotation_from_schema

* fix typo

* rename _schema to schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants