8000 Added schema generation for Generic fields by maximberg · Pull Request #2262 · pydantic/pydantic · GitHub
[go: up one dir, main page]
Skip to content

Added schema generation for Generic fields #2262

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.

Sign up for GitHub

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

Jump to bottom
Merged
samuelcolvin merged 14 commits into from
Feb 13, 2021
Merged

Added schema generation for Generic fields #2262

samuelcolvin merged 14 commits into from
Feb 13, 2021

Conversation

@maximberg
Copy link
Contributor
@maximberg maximberg commented Jan 14, 2021 •
edited
Loading

Change Summary

Generating schema for generic fields hasn't been implemented. Now it should work.

Related issue number

Fixes #1578.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link
codecov bot commented Jan 14, 2021 •
edited
Loading

Codecov Report

Merging #2262 (bf9e4df) into master (13a5c7d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2262   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         4199      4205    +6     
  Branches       854       856    +2     
=========================================
+ Hits          4199      4205    +6
Impacted Files Coverage Δ
pydantic/typing.py 100.00% <ø> (ø)
pydantic/schema.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a5c7d...bf9e4df. Read the comment docs.

@PrettyWood
Copy link
Collaborator
PrettyWood commented Jan 14, 2021

Hi @maximberg and thanks for the contribution.
I don't know if you saw but #2260 was opened yesterday and tackles the same issue.
Probably good to merge both works!

@maximberg
Copy link
Contributor Author
maximberg commented Jan 14, 2021 •
edited
Loading

Hi @PrettyWood,
I didn't see your pull request yesterday. See what you can add to mine if it's not difficult for you. Tests passed, but coverage decreased.
Thanks )

@maximberg
Copy link
Contributor Author
maximberg commented Jan 14, 2021

Coverage restored.

@dashavoo dashavoo mentioned this pull request Jan 14, 2021
Closed
4 tasks
Mazyod
Mazyod reviewed Jan 19, 2021
View reviewed changes
@PrettyWood
Copy link
Collaborator
PrettyWood commented Jan 19, 2021 •
edited
Loading

I didn't see your pull request yesterday. See what you can add to mine if it's not difficult for you. Tests passed, but coverage decreased.

As a matter of fact it's not my PR but @danni's. I just think both works should be merged into one. If this PR is good to go and test cases from @danni's PR are green with your PR fine! But I won't checkout locally both works and do it myself sorry

@maximberg @Mazyod
Update pydantic/schema.py
bf9e4df 
Co-authored-by: Maz Jaleel <mazjaleel@gmail.com>
@maximberg
Copy link
Contributor Author
maximberg commented Jan 21, 2021

@Mazyod Thanks! I used your proposal.

@maximberg
Copy link
Contributor Author
maximberg commented Jan 21, 2021 •
edited
Loading

@PrettyWood Actually @danni's PR is not exactly about problem, described in #1578.

@Mazyod
Copy link
Contributor
Mazyod commented Feb 5, 2021

@samuelcolvin I wanted to sponsor this project because recent generics enhancements will make a big difference for our project 🙇‍♂️ Appreciate your help

@davidhyman
Copy link
davidhyman commented Feb 12, 2021

👍 here too

I have tried out both #2262 (this) and #2260 MRs and they both work well with my codebase 😄

Currently @danni's MR is indicating lower code coverage. However, two of those tests flag failures on @maximberg's MR (test_nested_generic, test_complex_nested_generic), and I see (but don't fully comprehend) the diff between them.

By way of expediency, perhaps this MR could be merged as-is, providing an improvement? A later MR would then add the nested generic functionality.

In the meantime I will attempt to understand both MRs...

@maximberg
Copy link
Contributor Author
maximberg commented Feb 12, 2021

@davidhyman My code solves the problem of generic fields in generating schema.
@danni tries to solve the problem of nested generic models.
In my project I want to use generic fields and I made necessary changes here.

@maximberg
Copy link
Contributor Author
maximberg commented Feb 12, 2021 •
edited
Loading

I found some strange behavior when generic field is Optional. Will check it on Monday, 15th.

@samuelcolvin samuelcolvin merged commit c3870b6 into pydantic:master Feb 13, 2021
@samuelcolvin
Copy link
Member
samuelcolvin commented Feb 13, 2021

this looks great, thank you.

If you find other strange behaviour, please create an issue or if you're confident, a PR.

@samuelcolvin samuelcolvin mentioned this pull request Feb 13, 2021
Closed
4 tasks
@maximberg maximberg mentioned this pull request Feb 17, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Mazyod Mazyod Mazyod left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BaseModel.schema() fails when using generic types

5 participants

@maximberg @PrettyWood @Mazyod @davidhyman @samuelcolvin
0