Conversation
Reviewer's GuideUpdates datamodel serialization to better control excluded fields and restores enum compatibility for Python 3.10+, while bumping the library version. Class diagram for updated datamodel BaseModel serialization methodsclassDiagram
class BaseModel {
+to_dict(remove_nulls bool = False, convert_enums bool = False, as_values bool = False, exclude set = None) dict~str, Any~
+__collapse_as_values__(remove_nulls bool = False, convert_enums bool = False, as_values bool = False, exclude set = None) dict~str, Any~
+columns() dict~str, Field~
}
class Field {
+metadata dict~str, Any~
}
BaseModel "*" --> "*" Field : uses
Flow diagram for updated to_dict and collapse_as_values exclusion logicflowchart TD
A[to_dict called] --> B[Initialize exclude set or use provided]
B --> C[Add _pgoutput to exclude]
C --> D{as_values?}
D -- Yes --> E[Call __collapse_as_values__ with remove_nulls, convert_enums, as_values, exclude]
D -- No --> F[Call as_dict to build dict]
F --> G{convert_enums?}
G -- Yes --> H[Convert enum values in dict]
G -- No --> I[Skip enum conversion]
H --> J[Optionally remove nulls]
I --> J[Optionally remove nulls]
J --> K[Return dict]
subgraph CollapseValues[__collapse_as_values__]
L[Iterate over columns] --> M{field name in exclude?}
M -- Yes --> L
M -- No --> N{field name starts with __ or equals __errors__?}
N -- Yes --> L
N -- No --> O{field.metadata.exclude is True?}
O -- Yes --> L
O -- No --> P[Get attribute value]
P --> Q[Apply remove_nulls / convert_enums / collapse logic]
Q --> R[Store in output dict]
R --> L
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
exclude = exclude or set(); exclude.add('_pgoutput')pattern into_dictmutates a caller-providedexcludeset in-place, which may be surprising; consider copying it before modification if you intendexcludeto be local to this call. - The new
excludehandling is only applied in the__collapse_as_values__branch ofto_dict, so_pgoutputand other excluded fields will still appear whenas_values=False; if the intent is to always exclude these fields, the non-as_valuespath should apply the same logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `exclude = exclude or set(); exclude.add('_pgoutput')` pattern in `to_dict` mutates a caller-provided `exclude` set in-place, which may be surprising; consider copying it before modification if you intend `exclude` to be local to this call.
- The new `exclude` handling is only applied in the `__collapse_as_values__` branch of `to_dict`, so `_pgoutput` and other excluded fields will still appear when `as_values=False`; if the intent is to always exclude these fields, the non-`as_values` path should apply the same logic.
## Individual Comments
### Comment 1
<location> `datamodel/models.py:214` </location>
<code_context>
convert_enums: bool = False,
- as_values: bool = False
+ as_values: bool = False,
+ exclude: Optional[set] = None
) -> dict[str, Any]:
+ exclude = exclude or set()
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid mutating the caller-provided `exclude` set inside `to_dict`.
Since `exclude` comes from the caller, `exclude = exclude or set(); exclude.add('_pgoutput')` will modify the caller’s set. If the same set is reused, it will unexpectedly keep accumulating entries like `_pgoutput`. Instead, copy it first, e.g. `exclude_local = set(exclude or ())`, then add `'_pgoutput'` to the copy so the function doesn’t mutate caller-provided data.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| convert_enums: bool = False, | ||
| as_values: bool = False | ||
| as_values: bool = False, | ||
| exclude: Optional[set] = None |
There was a problem hiding this comment.
issue (bug_risk): Avoid mutating the caller-provided exclude set inside to_dict.
Since exclude comes from the caller, exclude = exclude or set(); exclude.add('_pgoutput') will modify the caller’s set. If the same set is reused, it will unexpectedly keep accumulating entries like _pgoutput. Instead, copy it first, e.g. exclude_local = set(exclude or ()), then add '_pgoutput' to the copy so the function doesn’t mutate caller-provided data.
Summary by Sourcery
Ensure compatibility with Python 3.10 while improving model serialization behavior and bumping the library version.
Bug Fixes:
Enhancements: