New exports - #255
Merged
Merged
Conversation
Reviewer's Guide by SourceryThis pull request introduces several new features and improvements to the datamodel library, including enhanced handling of empty values, support for parsing Path objects, improved parsing logic for set, dict, list, tuple, Optional, and Union types, constraint validation for primitive types, and improved type checking and validation logic. It also adds helper functions to handle lists and sets of dataclasses, and modifies the Field class to include additional attributes. Sequence diagram for parsing a List[Dataclass]sequenceDiagram
participant Parser
participant _parse_list_type
participant DataclassConstructor
Parser->>_parse_list_type: _parse_typing(field, T, data, encoder, targs)
activate _parse_list_type
loop for each item in data
_parse_list_type->>DataclassConstructor: inner_type(**d) or inner_type(*d) or inner_type(d)
activate DataclassConstructor
DataclassConstructor-->>_parse_list_type: instance of inner_type
deactivate DataclassConstructor
end
_parse_list_type-->>Parser: result
deactivate _parse_list_type
Sequence diagram for parsing a Union typesequenceDiagram
participant Parser
participant _parse_union_type
participant _parse_type
Parser->>_parse_union_type: _parse_typing(field, T, data, encoder, as_objects)
activate _parse_union_type
loop for each arg_type in targs
_parse_union_type->>_parse_type: _parse_type(field, arg_type, data, encoder, False)
activate _parse_type
_parse_type-->>_parse_union_type: result
deactivate _parse_type
end
_parse_union_type-->>Parser: result
deactivate _parse_union_type
Sequence diagram for parsing a Tuple typesequenceDiagram
participant Parser
participant _parse_tuple_type
participant _parse_type
Parser->>_parse_tuple_type: _parse_typing(field, T, data, encoder, as_objects)
activate _parse_tuple_type
alt Homogeneous Tuple
loop for each item in data
_parse_tuple_type->>_parse_type: _parse_type(field, element_type, item, encoder, False)
activate _parse_type
_parse_type-->>_parse_tuple_type: converted_item
deactivate _parse_type
end
else Heterogeneous Tuple
loop for each item_type, item in zip(args, data)
_parse_tuple_type->>_parse_type: _parse_type(field, item_type, item, encoder, False)
activate _parse_type
_parse_type-->>_parse_tuple_type: converted_item
deactivate _parse_type
end
end
_parse_tuple_type-->>Parser: result
deactivate _parse_tuple_type
Updated class diagram for FieldclassDiagram
class Field {
-compare
-init
-repr
-default
-default_factory
-metadata
-alias
-name
-field_name
-field_type
-required
-nullable
-validator
-parser
-encoder
-primary_key
-unique
-index
-db_default
-label
-description
-fk
-api
-multiple
-strict
-frozen
-remove_nulls
-no_nesting
-as_objects
-type
-type_args
-origin
-args
-_key_type
-_value_type
-_key_encoder_fn
-_value_encoder_fn
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @phenobarbital - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more specific error messages in the
_parse_*_typefunctions to provide better context when parsing fails. - The
is_emptyfunction could be simplified by directly returning the result of the boolean expressions, rather than assigning to a variable and returning it.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+75
to
+78
| if PyObject_IsInstance(value, (tuple, list, set)) and len(value) == 0: | ||
| return True | ||
|
|
||
| # Special case for containers: empty containers are NOT considered empty |
There was a problem hiding this comment.
suggestion: Clarify conflicting empty container checks.
The logic first returns True when a container (tuple, list, set) is empty based on its length, but then unconditionally returns False for any container of these types. It may help to review and consolidate these conditions to ensure that the method’s intent is unambiguous.
Suggested implementation:
# Special case for containers (tuple, list, set): they are valid initialized values,
# even if they are empty, so they are not considered empty.
if PyObject_IsInstance(value, (list, tuple, set)):
return False
Comment on lines
+267
to
271
| elif _type is set or _type is frozenset: | ||
| _type_category = 'set' | ||
| elif _is_dc: | ||
| _type_category = 'dataclass' | ||
| elif _is_typing or _is_alias: # noqa |
There was a problem hiding this comment.
suggestion: Field initialization for container types looks complete.
The modifications to handle 'set', 'frozenset', and tuple types (including caching inner types and encoder functions) add useful functionality. Please ensure that any default behaviors (e.g. using Any when no type argument is provided) are fully documented so that the expectations for these fields remain clear.
Suggested implementation:
elif _type is set or _type is frozenset:
_type_category = 'set' # Default: if no inner type is provided for a set/frozenset, it will be interpreted as Any. # Default behavior: if the container type has no explicit type argument, default to Any.
df._inner_type = args[0] if args else AnyVerify that similar inline documentation is added to other container type handlers (such as for tuple types) if they are defined elsewhere in the file. This will ensure that the expectations for default behaviors are consistent and clearly documented.
Comment on lines
+56
to
+59
| # Basic fields | ||
| id: int | ||
| uid: uuid.UUID = Field(primary_key=True, default=auto_uuid) | ||
| name: str = 'John Doe' |
There was a problem hiding this comment.
suggestion (testing): Missing edge case tests for basic fields
Consider adding tests for edge cases like empty strings for name, and invalid types for id (e.g., passing a string instead of an integer).
Suggested implementation:
import pytest
from pydantic import ValidationError
from your_module import User # Adjust the import based on your project structure
def test_empty_name():
"""
Test that an empty string for the 'name' field is accepted.
"""
user = User(id=1, name="")
assert user.name == "", "User name should accept an empty string"
def test_invalid_id_type_raises_error():
"""
Test that providing a non-integer for the 'id' field raises a ValidationError.
"""
with pytest.raises(ValidationError):
User(id="invalid", name="John Doe")Ensure that the module containing the User class is correctly imported in the test file. Adjust the import statement ('from your_module import User') to match your project's structure.
Comment on lines
+64
to
+66
| # Generic alias fields | ||
| friends: List[int] = Field(default_factory=list) # Typed list | ||
| roles: list = Field(default_factory=list) # Bare list |
There was a problem hiding this comment.
suggestion (testing): Missing tests for empty lists
It's important to test how the model behaves with empty lists for friends and roles. Add tests where these fields are initialized as empty lists.
Suggested implementation:
import pytest
from datetime import datetime, date
from typing import List, Optional, Dict, Tuple
import uuid
# Import the model that contains the fields under test.
# Replace 'YourModel' with the actual model name if different.
from app.models import YourModel, Address, Account # adjust import path if needed
def test_empty_lists():
# Create an instance with defaults for friends and roles.
model = YourModel(
id=1,
name="Test User",
uid=uuid.uuid4()
)
# Test that friends and roles are empty lists.
assert model.friends == [], "Expected 'friends' field to be an empty list by default"
assert model.roles == [], "Expected 'roles' field to be an empty list by default"Ensure that:
- The import path and model name ('YourModel') match the actual model defined in your codebase.
- Additional configuration may be required if the model uses other mandatory fields.
Comment on lines
+32
to
+35
| # Union types with containers | ||
| union_list: Union[List[int], str] = Field(default=None) | ||
| union_dict: Union[Dict[str, int], int] = Field(default=None) | ||
| union_set: Union[Set[str], str] = Field(default=None) |
There was a problem hiding this comment.
suggestion (testing): Missing negative tests for Union types
Add tests where incorrect types are passed to the union fields (e.g., an integer to union_list, a string to union_dict, etc.) to verify that validation errors are raised as expected.
Suggested implementation:
import pytest
from pydantic import ValidationError
# Adjust the import below to match the location and name of your model
from your_model_file import ContainerModel
def test_invalid_union_list():
# union_list expects either a list[int] or a str, here an int is invalid
with pytest.raises(ValidationError):
ContainerModel(union_list=123)
def test_invalid_union_dict():
# union_dict expects either a dict[str, int] or an int, here a str is invalid
with pytest.raises(ValidationError):
ContainerModel(union_dict="invalid")
def test_invalid_union_set():
# union_set expects either a set[str] or a str, here an int is invalid
with pytest.raises(ValidationError):
ContainerModel(union_set=42)If you haven't defined a model named ContainerModel, please update the import in the tests to use the correct model name. Also, adjust the file path in the import to reflect your actual project structure.
| except (TypeError, KeyError): | ||
| df._encoder_fn = None | ||
| # Handle list type | ||
| if origin is list: |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the container type handling logic into helper functions and using a dispatch table to reduce code duplication and improve readability, while keeping the Union case separate due to its unique handling requirements.
Consider extracting the duplicate logic for container types into helper functions or a dispatch table. This refactoring will reduce complexity by removing repeated try/except blocks and similar assignments. For example:
def _process_container(df, args):
df._inner_targs = df.args
df._inner_type = args[0] if args else Any
try:
df._encoder_fn = encoders[df._inner_type]
except (TypeError, KeyError):
df._encoder_fn = None
def _process_tuple(df, args):
df._inner_targs = df.args
has_ellipsis = len(args) == 2 and args[1] is Ellipsis
df._inner_type = args[0] if (has_ellipsis or args) else Any
try:
df._encoder_fn = encoders[df._inner_type]
except (TypeError, KeyError):
df._encoder_fn = None
def _process_dict(df, args):
df._inner_targs = df.args
df._key_type = args[0] if len(args) > 0 else Any
df._value_type = args[1] if len(args) > 1 else Any
df._inner_origin = get_origin(df._value_type)
df._typing_args = get_args(df._value_type)
try:
df._key_encoder_fn = encoders[df._key_type]
except (TypeError, KeyError):
df._key_encoder_fn = None
try:
df._value_encoder_fn = encoders[df._value_type]
except (TypeError, KeyError):
df._value_encoder_fn = None
# Create a dispatch table mapping origin to the corresponding processor function
_container_processors = {
list: _process_container,
set: _process_container,
tuple: _process_tuple,
dict: _process_dict,
}
# In your main flow under _is_typing or _is_alias branch:
if origin in _container_processors:
_container_processors[origin](df, args)
elif origin is Union:
df._inner_targs = df.args
try:
df._inner_type = args[0]
except IndexError as exc:
raise IndexError(
f"Union type {field} must have at least one type."
) from exc
df._inner_is_dc = is_dataclass(df._inner_type)
df._inner_priv = is_primitive(df._inner_type)
df._inner_origin = get_origin(df._inner_type)
df._typing_args = get_args(df._inner_type)Action steps:
- Move the common assignment and try/except blocks into helper functions (_process_container, _process_tuple, _process_dict).
- Replace the repetitive if/elif branches with a dispatch table lookup using the container's origin.
- Retain the Union case separately since it has unique handling.
This refactoring maintains functionality while reducing code duplication and complexity.
Comment on lines
+344
to
+353
| for test_name, test_fn in validation_tests: | ||
| print(f"Testing: {test_name}") | ||
| try: | ||
| test_fn() | ||
| print(" ERROR: Validation should have failed but it passed!") | ||
| except ValidationError as e: | ||
| print(f" Validation failed as expected: {e.payload}") | ||
| except Exception as e: | ||
| print(f" Unexpected error: {e}") | ||
| print() |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Comment on lines
+65
to
+68
| def validate_username_not_admin(field, value, annotated_type, val_type): | ||
| if value.lower() == 'admin': | ||
| return False | ||
| return True |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- The first argument to instance methods should be
self(instance-method-first-arg-name) - Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp) - Simplify boolean if expression (
boolean-if-exp-identity) - Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast)
Suggested change
| def validate_username_not_admin(field, value, annotated_type, val_type): | |
| if value.lower() == 'admin': | |
| return False | |
| return True | |
| def validate_username_not_admin(self, value, annotated_type, val_type): | |
| return value.lower() != 'admin' |
Comment on lines
+162
to
+166
| def validate_restricted_name(field, value, annotated_type, val_type): | ||
| restricted = ["sample", "test", "dummy", "placeholder"] | ||
| if value.lower() in restricted: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- The first argument to instance methods should be
self(instance-method-first-arg-name) - Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp) - Simplify boolean if expression (
boolean-if-exp-identity) - Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast)
Comment on lines
+176
to
+183
| model = ContainerTypesModel(typed_list=["1", "2", "3"]) # Strings should convert to ints | ||
| assert all(isinstance(x, int) for x in model.typed_list) | ||
| assert model.typed_list == [1, 2, 3] | ||
|
|
||
| # Test with mixed string/int input that should convert properly | ||
| model = ContainerTypesModel(typed_list=[1, "2", 3]) | ||
| assert all(isinstance(x, int) for x in model.typed_list) | ||
| assert model.typed_list == [1, 2, 3] |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enhance type parsing and validation for container types in the datamodel library, with improved support for generics, type conversion, and constraint validation
New Features:
Bug Fixes:
Enhancements:
Tests: