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

fix: do not ignore Config fields information in dataclass #2437

Closed

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Mar 1, 2021

Change Summary

We used to keep only the default value and the type when creating a dataclass, which would then follow the default workflow. But now we can use metadata in dataclasses.field or directly a pydantic.Field instance. We hence ignore the information set in the Config

Related issue number

Closes #2426

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 Mar 1, 2021

Codecov Report

Merging #2437 (ac21529) into master (a8d50ae) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2437   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5080      5085    +5     
  Branches      1041      1042    +1     
=========================================
+ Hits          5080      5085    +5     
Impacted Files Coverage Δ
pydantic/dataclasses.py 100.00% <100.00%> (ø)
pydantic/main.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 a8d50ae...ac21529. Read the comment docs.

@samuelcolvin
Copy link
Member

Would this also fix #2424?

@PrettyWood
Copy link
Member Author

Unfortunetaly no. #2424 seems trickier.

@samuelcolvin samuelcolvin added this to the v1.8.1 milestone Mar 2, 2021
field_info = Field(
default=default,
default_factory=default_factory,
**config_instance.get_field_info(field.name),
Copy link
Member

Choose a reason for hiding this comment

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

Humm, I think this could lead to unexpected behaviour - get_field_info is not designed to return kwargs for Field, I think it's not used for this anywhere else.

Perhaps we could add some tests of using get_field_info to create Fields and update the docstring?

Copy link
Member

Choose a reason for hiding this comment

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

But in theory this should all be done when creating the field as it is normally, maybe we should work out why it's not?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is really an issue of merging infos from config to an instance of Field. As a matter of fact it's the same with create_model

from pydantic import create_model, Field

class Config:
    fields = {'a': {'description': 'descr'}}

Ad = create_model('Ad', __config__=Config, a=(str, ...))
assert Ad.schema()['properties'] == {'a': {'title': 'A', 'description': 'descr', 'type': 'string'}}

Ad = create_model('Ad', __config__=Config, a=(str, Field(...)))
assert Ad.schema()['properties'] == {'a': {'title': 'A', 'type': 'string'}}

@samuelcolvin I would suggest we don't include this in 1.8.1 and consider this as a classic "bug".
We can then take time to refactor calmly some stuff if needed as you suggested

Copy link
Member

Choose a reason for hiding this comment

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

humm, I understand what you mean, but I think given that it's new since 1.8, we should try to fix it if possible.

I will look into it now.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out the same thing even happens with a vanilla pydantic model:

from pydantic import Field, BaseModel

class Foo(BaseModel):
    a: str = Field(...)

    class Config:
        fields = {'a': {'description': 'descr'}}

debug(Foo.schema()['properties'])

I'll look into it now.

This is not new in 1.8, it was the same in v1.7.3. Still, should be fixed.

@samuelcolvin
Copy link
Member

superseded by #2461

@PrettyWood PrettyWood deleted the dataclass-config-field branch March 3, 2021 16:58
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.

dataclass config fields not warranted in json schema (1.8)
2 participants