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

Add support for JSON Schema with circular references in Python 3.7 #572

Merged

Conversation

@tiangolo
Copy link
Sponsor Collaborator

@tiangolo tiangolo commented Jun 3, 2019

Change Summary

Add JSON Schema generation support for models with circular references, including self-referencing.

Related issue number

This fixes #531, the comment in #524 (comment).

I think parts of it might move in the direction of #478 too.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>
@codecov
Copy link

@codecov codecov bot commented Jun 3, 2019

Codecov Report

Merging #572 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #572   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2498   2508   +10     
  Branches      499    501    +2     
=====================================
+ Hits         2498   2508   +10

@tiangolo
Copy link
Sponsor Collaborator Author

@tiangolo tiangolo commented Jun 3, 2019

There's something strange happening to GitHub.

The tests finished a while ago, but they still show up as "running".

The same for Travis and for the tests in Netlify. And the coverage was raise again, removing the partial, but the comment still hasn't been updated.

pydantic/schema.py Outdated Show resolved Hide resolved
pydantic/schema.py Outdated Show resolved Hide resolved
pydantic/schema.py Outdated Show resolved Hide resolved
pydantic/schema.py Outdated Show resolved Hide resolved
pydantic/schema.py Outdated Show resolved Hide resolved
@@ -662,6 +691,7 @@ def field_singleton_schema( # noqa: C901 (ignore complexity)
model_name_map: Dict[Type['BaseModel'], str],
schema_overrides: bool = False,
ref_prefix: Optional[str] = None,
known_models: Set[Type['BaseModel']],
Copy link
Owner

@samuelcolvin samuelcolvin Jun 4, 2019

Choose a reason for hiding this comment

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

this function is MASSIVE is there anything we can do to simplify it?

Copy link
Collaborator Author

@tiangolo tiangolo Jun 4, 2019

Choose a reason for hiding this comment

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

Let me check what I can do...

Copy link
Owner

@samuelcolvin samuelcolvin Jun 4, 2019

Choose a reason for hiding this comment

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

if it's not possible or too difficult, don't worry. Maybe better to do in a separate PR anyway.

Copy link
Collaborator Author

@tiangolo tiangolo Jun 4, 2019

Choose a reason for hiding this comment

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

Hmm, I agree it's big. But I don't really know what to put outside, I don't see much code repetition as it handles several different cases.

Maybe the last section that checks if it's a model, under if issubclass(field_type, pydantic.BaseModel): could be in a function...

But it would require several parameters, would have to modify some of them (which is a bit difficult to debug/refactor later) and would require an extra if to check if it returned a value (to return it) or not, so I'm not sure.

Do you want me to put it outside? Or later?

Copy link
Owner

@samuelcolvin samuelcolvin Jun 4, 2019

Choose a reason for hiding this comment

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

let's do it (or not do it) later.

tests/test_py37.py Outdated Show resolved Hide resolved
tests/test_py37.py Outdated Show resolved Hide resolved
tests/test_schema.py Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin assigned samuelcolvin and unassigned tiangolo Jun 4, 2019
@samuelcolvin samuelcolvin merged commit d73aa1b into samuelcolvin:master Jun 4, 2019
8 checks passed
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 4, 2019

awesome. Thank you.

@tiangolo
Copy link
Sponsor Collaborator Author

@tiangolo tiangolo commented Jun 4, 2019

Great! Thanks 🎉

@tiangolo tiangolo deleted the json-schema-circular-references branch Jun 4, 2019
@samuelcolvin samuelcolvin mentioned this pull request Jun 6, 2019
4 tasks
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.

2 participants