Skip to content

Conversation

tiangolo
Copy link
Contributor

@tiangolo tiangolo commented Nov 25, 2019

Change Summary

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

This allows solving fastapi/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
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 changed the title WIP: Implement Optional required Implement Optional required Nov 26, 2019
@tiangolo
Copy link
Contributor Author

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
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise looking good.

@tiangolo
Copy link
Contributor Author

Thanks for the code review! Done ✔️

@samuelcolvin
Copy link
Member

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
Copy link
Contributor Author

Sure! 📝 Done ✔️

@dmontagu
Copy link
Contributor

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
Copy link
Contributor 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?

@dmontagu
Copy link
Contributor

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
Copy link
Contributor Author

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! 💪 😎 🚀

@tiangolo
Copy link
Contributor Author

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

@samuelcolvin samuelcolvin merged commit d9bbb05 into pydantic:master Nov 28, 2019
@samuelcolvin
Copy link
Member

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

@tiangolo
Copy link
Contributor Author

Thanks for fixing the tests I missed 🤦‍♂️

And thanks for merging this! 🎉 🚀

@tiangolo tiangolo deleted the optional-required branch November 28, 2019 17:02
andreshndz pushed a commit to cuenca-mx/pydantic that referenced this pull request Jan 17, 2020
* Implement Optional required, when creating a ModelField(required=True), make it persist

* Add test for nullable required

* Improve formatting of Undefined custom object

* Refactor field infer/creation with Undefined to make it idempotent

Needed for when _type_analysis is re-run in Generics

* Add PR changes

* Increment/update tests with code review

* Update/refactor Undefined implementation with code review

* Fix BoolUndefined as string type for mypy, not runtime

* Add docs about required Optional

* Add explicit tests for Any

* Apply code review requested changes

* move tests out of test_validators.py
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

3 participants