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

Clean up Field parameters #5425

Merged
merged 3 commits into from
Apr 11, 2023
Merged

Clean up Field parameters #5425

merged 3 commits into from
Apr 11, 2023

Conversation

hramezani
Copy link
Member

@hramezani hramezani commented Apr 10, 2023 •
edited by pydantic-hooky bot
Loading

Change Summary

Clean up Field parameters

Related issue number

Fixes #5406

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)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

@hramezani
Copy link
Member Author

please review

samuelcolvin
samuelcolvin reviewed Apr 10, 2023
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.

Might be work asking @dmontagu about why he made the default of frozen=None.

please update.

pydantic/fields.py Outdated
@@ -375,16 +379,18 @@ def Field(
of digits within the decimal. It does not include a zero before the decimal point or trailing decimal zeroes.
:param decimal_places: only applies to Decimals, requires the field to have at most a number of decimal places
allowed. It does not include trailing decimal zeroes.
:param min_items: only applies to lists, requires the field to have a minimum number of
:param min_items: deprecated, use ``min_length`` instead.
Copy link
Member

Choose a reason for hiding this comment

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

we want to use single back-ticks since this will be rendered as markdown - might as well fix the whole docstring now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed for all docstrings in code

pydantic/fields.py Outdated
# Check deprecated & removed params of V1.
# This has to be removed deprecation period over.
if const:
raise ValueError('`const` is removed. use `Literal` instead')
Copy link
Member

Choose a reason for hiding this comment

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

Please use PydanticUserError, add a code and a section to docs if you think necessary, otherwise set code=None.

Copy link
Member Author

@hramezani hramezani Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done. field-removed-param error code defined

pydantic/fields.py Outdated
raise ValueError('`const` is removed. use `Literal` instead')
if min_items:
warn('`min_items` is deprecated and will be removed. use `min_length` instead', DeprecationWarning)
min_length = min_items
Copy link
Member

Choose a reason for hiding this comment

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

if min_length is None.

Copy link
Member

Choose a reason for hiding this comment

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

and below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. done

pydantic/fields.py Outdated
warn('`max_items` is deprecated and will be removed. use `max_length` instead', DeprecationWarning)
max_length = max_items
if unique_items:
raise ValueError('`unique_items` is removed. use `Set` instead')
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
raise ValueError('`unique_items` is removed. use `Set` instead')
raise ValueError('`unique_items` is removed, use `Set` instead')

Might be worth adding (this feature is discussed in https://github.com/pydantic/pydantic-core/issues/296)

pydantic/pydantic-core#296

tests/test_annotated.py Outdated
@@ -22,22 +22,22 @@
(
lambda: Annotated[int, Field(gt=0)],
5,
'FieldInfo(annotation=int, required=False, default=5, metadata=[Gt(gt=0)])',
'FieldInfo(annotation=int, required=False, default=5, metadata=[Gt(gt=0)], frozen=False)',
Copy link
Member

Choose a reason for hiding this comment

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

if we set the default of frozen to False, we should ignore frozen=False in repr.

Copy link
Member Author

Choose a reason for hiding this comment

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

ignored

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Apr 10, 2023
@pydantic-hooky pydantic-hooky bot assigned hramezani and unassigned samuelcolvin Apr 10, 2023
@hramezani
Copy link
Member Author

please review

@pydantic-hooky pydantic-hooky bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Apr 10, 2023
@pydantic-hooky pydantic-hooky bot assigned samuelcolvin and unassigned hramezani Apr 10, 2023
@dmontagu
Copy link
Contributor

Might be work asking @dmontagu about why he made the default of frozen=None.

I don't think this was super intentional.

I think I may have been following the pattern used for allow_inf_nan: bool | None = None, as far as I can tell that is also a case where perhaps the default should be a boolean rather than None. Unless None has different behavior than one of the booleans, my inclination would be to change both cases to use bool only.

And given the following snippet from the docstring, I think None doesn't have special behavior there:

:param allow_inf_nan: only applies to numbers, allows the field to be NaN or infinity (+inf or -inf),
        which is a valid Python float. Default True, set to False for compatibility with JSON.

samuelcolvin
samuelcolvin reviewed Apr 11, 2023
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.

please update.

pydantic/fields.py Outdated Show resolved Hide resolved
@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Apr 11, 2023
@pydantic-hooky pydantic-hooky bot assigned hramezani and unassigned samuelcolvin Apr 11, 2023
@hramezani
Copy link
Member Author

please review

@pydantic-hooky pydantic-hooky bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels Apr 11, 2023
@pydantic-hooky pydantic-hooky bot assigned samuelcolvin and unassigned hramezani Apr 11, 2023
@samuelcolvin samuelcolvin merged commit 1896500 into pydantic:main Apr 11, 2023
@samuelcolvin
Copy link
Member

this is great, thank you.

@hramezani hramezani deleted the clean_field_parameters branch April 11, 2023 15:58
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

@samuelcolvin samuelcolvin

Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clean up Field parameters
3 participants
@hramezani @dmontagu @samuelcolvin