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

Implement Optional required #1031

Merged
merged 13 commits into from Nov 28, 2019
Merged

Conversation

@tiangolo
Copy link
Collaborator

tiangolo commented Nov 25, 2019

Change Summary

Make ModelField(required=True) keep the required flag.

This allows solving tiangolo/fastapi#646 without hacks, and solves #990, allowing required nullables with:

class Model(BaseModel):
    nullable: Optional[int] = Field(...)

A benefit is that external libraries using Pydantic like FastAPI or others (and more to come) can create a ModelField object, set the required argument (ModelField(required=True)) and expect it to be preserved without having to re-set it after creation with field.required = True.

Related issue number

#990 #1028

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

This comment has been minimized.

Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #1031 into master will decrease coverage by 0.03%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #1031      +/-   ##
==========================================
- Coverage     100%   99.96%   -0.04%     
==========================================
  Files          20       20              
  Lines        3316     3332      +16     
  Branches      653      659       +6     
==========================================
+ Hits         3316     3331      +15     
- Misses          0        1       +1
Impacted Files Coverage Δ
pydantic/fields.py 99.71% <95.23%> (-0.29%) ⬇️

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 c71326d...723ddc7. Read the comment docs.

@tiangolo tiangolo requested review from dmontagu and samuelcolvin Nov 26, 2019
@tiangolo tiangolo changed the title WIP: Implement Optional required Implement Optional required Nov 26, 2019
@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 26, 2019

This is ready for review @samuelcolvin , @dmontagu .

I didn't think the optional nullable was worth extra docs, but let me know if you think this should include some.

Copy link
Owner

samuelcolvin left a comment

otherwise looking good.

tests/test_validators.py Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
pydantic/fields.py Show resolved Hide resolved
pydantic/fields.py Outdated Show resolved Hide resolved
pydantic/fields.py Outdated Show resolved Hide resolved
@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 26, 2019

Thanks for the code review! Done ✔️

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 26, 2019

Looks good, except on reflection we do need to update the docs.

We should extend this section to explain about required optional fields.

We should also add a note to this section pointing to the details on required optional fields.

@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 26, 2019

Sure! 📝 Done ✔️

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Nov 26, 2019

The mypy plugin may need to be changed to reflect this (either it needs to be, or it was a bug before 😅) I’ll take a look. Either way it won’t be hard.

@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 26, 2019

Thanks @dmontagu !

I haven't been able to check out the mypy plug-in (although I look forward to using it) 😅

Should we do the update here or in a separate PR?

tests/test_validators.py Outdated Show resolved Hide resolved
@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Nov 26, 2019

I looked at the mypy plugin code and it actually currently treats Optional[X] = ... as required 😄. So nothing to change there.

It would be nice to add tests for this, and I wrote some, but after some reflection I think it will be easiest to just put it in a separate PR (really need to make it easier to write unit tests for the mypy plugin...).

I'll take responsibility for writing additional mypy plugin tests, and I'll wait until after this is merged though and we make the desired behavior explicit.


I think it would be nice to add some tests here for Any too as noted above, just to make sure the logic is formalized somewhere. But that's the only thing left; if you or @samuelcolvin disagree about including those tests here, I'm good with this PR as is.

(Currently the plugin does not treat Any as non-required, so I guess as of v1.1 that's a bug that needs to be addressed; I'll fix that in the PR where I add the tests described above.)

@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 26, 2019

Thanks for the review @dmontagu !

I added tests for Any as you suggested. Including explicit tests for a: Any vs b: Any = None.

I guess that will have to be updated if the current behavior is changed.

Thanks for taking care of the mypy plug-in! 💪 😎 🚀

docs/usage/models.md Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
docs/usage/models.md Outdated Show resolved Hide resolved
docs/usage/models.md Outdated Show resolved Hide resolved
@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 28, 2019

Thanks for the review, I applied all the requested changes.

@samuelcolvin samuelcolvin merged commit d9bbb05 into samuelcolvin:master Nov 28, 2019
11 checks passed
11 checks passed
Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 99.96% (-0.04%) compared to c71326d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic #20191128.10 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
samuelcolvin.pydantic (Job Python38) Job Python38 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 28, 2019

thanks so much, I'll release v1.2 now.

@tiangolo

This comment has been minimized.

Copy link
Collaborator Author

tiangolo commented Nov 28, 2019

Thanks for fixing the tests I missed 🤦‍♂

And thanks for merging this! 🎉 🚀

@tiangolo tiangolo deleted the tiangolo:optional-required branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.