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.

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

fix(fields): handle properly default value for type Callable #2094

Merged
merged 8 commits into from Feb 11, 2021
Merged

fix(fields): handle properly default value for type Callable #2094

merged 8 commits into from Feb 11, 2021

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Nov 5, 2020 •
edited

Change Summary

from typing import Callable

from pydantic import BaseModel


def pika_default() -> str:
    return 'Pika is the best'

class PikaModel(BaseModel):
    pika: Callable[[], str] = pika_default


print(PikaModel.__fields__)

BEFORE

{}

AFTER

{'pika': ModelField(name='pika', type=Callable[[], str], required=False, default=<function pika_default at 0x10a6541f0>)}

Related issue number

closes #1596
closes #2086

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 5, 2020 •
edited

Codecov Report

Merging #2094 (904c2f5) into master (13a5c7d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2094   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         4199      4199           
  Branches       854       854           
=========================================
  Hits          4199      4199
Impacted Files Coverage Δ
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...904c2f5. Read the comment docs.

samuelcolvin
samuelcolvin reviewed Nov 29, 2020
View reviewed changes
pydantic/main.py Outdated
@@ -198,7 +198,8 @@ def validate_custom_root_type(fields: Dict[str, ModelField]) -> None:
raise ValueError('__root__ cannot be mixed with other fields')


UNTOUCHED_TYPES = FunctionType, property, type, classmethod, staticmethod
FIELD_DEFAULT_VALUE_UNTOUCHED_TYPES = property, type, classmethod, staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

can we make this name shorter. I think we need to describe what's going in here in a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment and renamed it to ANNOTATED_FIELD_UNTOUCHED_TYPES. Still a bit long but I couldn't find an explicit name shorter than that.

tests/test_main.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
samuelcolvin
samuelcolvin reviewed Dec 31, 2020
View reviewed changes
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 LGTM

tests/test_dataclasses.py Outdated
def non_default_callable(x):
return foo(x, 'nondefault')

for cls in (HasCallablesModel, HasCallablesDC):
Copy link
Member

Choose a reason for hiding this comment

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

it's really confusing if something fails to work out which object failed.

Better just to split the test and duplicate code, or if there's a lot of duplication, use fixtures.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the test from the issue but I agree it was a bit confusing. I rewrote the test. Hope it's easier to read.
Also added with an annotated callable like Callable[[int, float, str], bool].
Ready for another review :)

tests/test_dataclasses.py Outdated
default_callable_factory: Callable = dataclasses.field(default=lambda x: foo(x, 'factory'))

class HasCallablesModel(BaseModel):
non_default_callable: Callable
Copy link
Member

Choose a reason for hiding this comment

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

what about typed callables like Callable[[int, float, str], bool]?

Surely worth adding tests for them too?

@samuelcolvin samuelcolvin mentioned this pull request Jan 1, 2021
Merged
4 tasks
@PrettyWood
Copy link
Member Author

Added a small commit for schema generation in the batch since it's related (callable fields with default value) even though it's not the same root cause 😉

@PrettyWood PrettyWood mentioned this pull request Feb 2, 2021
Closed
3 tasks
@samuelcolvin samuelcolvin merged commit d0baf0f into pydantic:master Feb 11, 2021
@PrettyWood PrettyWood deleted the fix/callable-default-value branch February 11, 2021 12:09
@aminalaee aminalaee mentioned this pull request Jun 11, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

Assignees
No one assigned
Labels
ready for review
Projects
None yet
Milestone
No milestone
2 participants
@PrettyWood @samuelcolvin