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

Dynamic default value #1210

Merged
merged 5 commits into from Mar 4, 2020

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Feb 6, 2020

Change Summary

Following #155 and #866, here is a proposal of implementation to simplify dynamic default values

Right now, to have a dynamic default value, we need to use validators like

class DynamicValueModel(BaseModel):
    ts: datetime = None

    @validator('ts', pre=True, always=True)
        def set_ts_now(cls, v):
            return v or datetime.now()

Now, we can use default_factory (same name as dataclasses.to be consistent)

class DynamicValueModel(BaseModel):
    ts: datetime = Field(default_factory=datetime.now)

Related issue number

#155 and #866

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 Feb 6, 2020

Codecov Report

Merging #1210 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1210   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          21       21           
  Lines        3689     3706   +17     
  Branches      726      730    +4     
=======================================
+ Hits         3685     3702   +17     
  Misses          2        2           
  Partials        2        2           

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 0f94861...606449f. Read the comment docs.

@Bobronium
Copy link
Contributor

Bobronium commented Feb 6, 2020

Discussed in #866

So I would like to see this:

from datetime import datetime

from pydantic import BaseModel

class Record(BaseModel):
    name: str
    description: str
    ts: datetime = datetime.now

record = Record(name='Foo', description='Bar and Baz also.')
print(record.ts)
#> 2020-02-06 22:11:43.454720

However if type of field is callable, it must remain untouched:

from random import randint
from typing import Callable

from pydantic import BaseModel

class IntGenerator(BaseModel):
    fabric: Callable[..., int] = randint

generator = IntGenerator()
print(generator.fabric(0, 1))
#> 1

@samuelcolvin, any thoughts?

@PrettyWood
Copy link
Member Author

PrettyWood commented Feb 6, 2020

@MrMrRobat Thanks ! I thought about the implementation with Callable but I'm afraid it would maybe break some existing behaviour. I personally thought it was the most intuitive one at first but the more I think about it, the more I fear there are some cases where it won't be very clear and we may have some bad surprises.
This is why I chose to use an extra field. I will rename it to default_factory to be consistent with native dataclasses.
If the Callable approach is chosen, this PR can be closed

@PrettyWood PrettyWood force-pushed the dynamic-default-values branch 2 times, most recently from ba4603a to 5f7fc5d Compare February 7, 2020 03:00
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.

Mostly looks goods, needs tests fixing and docs.

@@ -76,8 +77,12 @@ class FieldInfo(Representation):
'extra',
)

def __init__(self, default: Any, **kwargs: Any) -> None:
def __init__(self, default: Any = Undefined, *, default_factory: Any = None, **kwargs: Any) -> None:
if default is not Undefined and default_factory is not None:
Copy link
Member

Choose a reason for hiding this comment

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

perhaps better to move this check into Fieldm then you can use pop() as is done elsewhere.

*,
default_factory: Any = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
default_factory: Any = None,
default_factory: Optional[Callable[[], Any]] = None,

or something

@samuelcolvin
Copy link
Member

I'm keen on the approach of

from datetime import datetime

from pydantic import BaseModel

class Record(BaseModel):
    ts: datetime = datetime.now

However I think it could cause subtle changes in behaviour in edge cases so let's wait for v2.

Getting Field(default_factory=...) to work now is a great first step.

@PrettyWood PrettyWood force-pushed the dynamic-default-values branch 2 times, most recently from 6b24d27 to 065cf78 Compare February 8, 2020 17:37
# With a callable: we still should be able to set callables as defaults
class FunctionModel(BaseModel):
a: int = 1
uid: Callable[[], UUID] = Field(uuid4)
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to use = Field(uuid) instead of = uuid4 as functions are ignored when creating fields and the default is hence not properly set
I think this behaviour should be fixed but in another PR (maybe for v2 with the other syntax for dynamic values)

*,
default_factory: Optional['Callable[[], Any]'] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to use 'Callable...' because it would raise
TypeError: 'ABCMeta' object is not subscriptable

Copy link
Contributor

Choose a reason for hiding this comment

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

How is that possible? It already used in this module without an issues. That could be the case for collections.Callable, but it's a different thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right ! I didnt' see the Callable came from .typing. Thanks !

@PrettyWood PrettyWood force-pushed the dynamic-default-values branch 2 times, most recently from 96fa43c to 5b08aef Compare February 10, 2020 10:09
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.

There's a chance we might need to change the signature or behaviour of default_factory in future.

Therefore might be useful to comment in the documentation that this feature is provisional and might change in future minor releases? See #1179 for an example.

Otherwise looking good.

@@ -0,0 +1 @@
Add `default_factory` `Field` argument to create a dynamic default value by passing a zero-argument callable.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add `default_factory` `Field` argument to create a dynamic default value by passing a zero-argument callable.
Add `default_factory` argument to `Field` to create a dynamic default value by passing a zero-argument callable.

@@ -3,6 +3,7 @@
from typing import (
TYPE_CHECKING,
Any,
Callable,
Copy link
Member

Choose a reason for hiding this comment

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

if the below is TypingCallable, what is this? Maybe good to give this an alias.

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 decided to add a NoArgAnyCallable directly in the local typing module. Makes things clearer imo

@@ -307,8 +321,9 @@ def prepare(self) -> None:
Note: this method is **not** idempotent (because _type_analysis is not idempotent),
e.g. calling it it multiple times may modify the field and configure it incorrectly.
"""
if self.default is not None and self.type_ is None:
self.type_ = type(self.default)
default_value = self.default_factory() if self.default_factory is not None else self.default
Copy link
Member

Choose a reason for hiding this comment

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

perhaps useful to have a get_default() function on this class which is used here and in main.py and perhaps deals with deepcopy?

@PrettyWood
Copy link
Member Author

@samuelcolvin Thanks for the review ! Changes done

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 looks good, sorry I missed this before.

pydantic/fields.py Outdated Show resolved Hide resolved
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@samuelcolvin samuelcolvin merged commit 78a3f42 into pydantic:master Mar 4, 2020
@jacobsvante
Copy link

@samuelcolvin When do you think a release will be ready with this PR? Looking forward to it!

@PrettyWood PrettyWood deleted the dynamic-default-values branch March 25, 2020 08:56
@samuelcolvin
Copy link
Member

@jmagnusson see #1341 for details on release date.

@keu keu mentioned this pull request Oct 1, 2021
38 tasks
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.

None yet

4 participants