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

added new variable that override pydantic fields definition with strawberry fields #3469

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShtykovaAA
Copy link

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@ShtykovaAA ShtykovaAA marked this pull request as ready for review April 22, 2024 07:17
@botberry
Copy link
Member

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

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:

🆕 Release (next) is out! Thanks to Shtykova Anna for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

@ShtykovaAA ShtykovaAA marked this pull request as draft April 22, 2024 07:19
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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.
"""

Comment on lines +951 to +969
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +951 to +969
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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.

2 participants