Skip to content

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

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
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
Collaborator

@PrettyWood PrettyWood commented Nov 5, 2020 •
edited
Loading

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
Loading

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
Collaborator 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.

@PrettyWood PrettyWood force-pushed the fix/callable-default-value branch from 1e75ab1 to cfba22c Compare November 29, 2020 22:36
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
Collaborator 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
Collaborator 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