added new variable that override pydantic fields definition with strawberry fields #3469
Conversation
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ShtykovaAA - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -64,16 +64,16 @@ def _build_dataclass_creation_fields( | |||
| auto_fields_set: Set[str], | |||
| use_pydantic_alias: bool, | |||
| compat: PydanticCompat, | |||
| override: bool = True, | |||
There was a problem hiding this comment.
suggestion (code_clarification): Consider documenting the purpose and usage of the 'override' parameter.
Adding a new parameter with a default value is a significant change. It would be beneficial to include inline documentation or update the function's docstring to explain how this parameter affects the function's behavior.
| override: bool = True, | |
| override: bool = True, | |
| """ | |
| :param override: A boolean flag that determines whether the default settings should be overridden. | |
| When set to True, the function will override the existing settings with the provided values. | |
| Defaults to True. | |
| """ |
| def test_can_override_pydantic_with_strawberry_definition(): | ||
| # Set variable override=False to pydantic type | ||
| # That's override pydantic fields with strawberry fields | ||
| class UserModel(BaseModel): | ||
| new_name: Optional[str] = None | ||
| new_age: Optional[int] = None | ||
|
|
||
| @strawberry.experimental.pydantic.type(UserModel, override=False) | ||
| class User: | ||
| new_name: Optional[str] = None | ||
| new_age: Optional[int] = strawberry.UNSET | ||
|
|
||
| origin_user = UserModel() | ||
| user = User() | ||
| assert origin_user.new_name is None | ||
| assert origin_user.new_age is None | ||
|
|
||
| assert user.new_name is None | ||
| assert user.new_age is strawberry.UNSET |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for when override=True.
The current test only covers the scenario where override=False. It would be beneficial to ensure that the default behavior (override=True) works as expected, especially since this is a new feature.
| def test_can_override_pydantic_with_strawberry_definition(): | ||
| # Set variable override=False to pydantic type | ||
| # That's override pydantic fields with strawberry fields | ||
| class UserModel(BaseModel): | ||
| new_name: Optional[str] = None | ||
| new_age: Optional[int] = None | ||
|
|
||
| @strawberry.experimental.pydantic.type(UserModel, override=False) | ||
| class User: | ||
| new_name: Optional[str] = None | ||
| new_age: Optional[int] = strawberry.UNSET | ||
|
|
||
| origin_user = UserModel() | ||
| user = User() | ||
| assert origin_user.new_name is None | ||
| assert origin_user.new_age is None | ||
|
|
||
| assert user.new_name is None | ||
| assert user.new_age is strawberry.UNSET |
There was a problem hiding this comment.
suggestion (testing): Add assertions to verify that the User fields correctly override the UserModel fields.
While the test checks the initial state of UserModel and User, it does not verify if the User fields actually override those from UserModel when set. This is crucial for validating the feature's functionality.
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist