-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: replace Pydantic BaseModel with dataclasses #73
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to d852bb0 in 1 minute and 56 seconds. Click for details.
- Reviewed
459lines of code in4files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/insights/schema.py:19
- Draft comment:
If an empty dict should be the default for 'metadata', use field(default_factory=dict) to avoid issues with mutable defaults. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/datapilot/core/platforms/dbt/schemas/catalog.py:17
- Draft comment:
'generated_at' is annotated as Optional[datetime] but its default is a string. Convert the default to a datetime (or adjust the type) for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/datapilot/core/platforms/dbt/schemas/catalog.py:27
- Draft comment:
Avoid using nested Optional types (e.g., Optional[Optional[str]]). Simplify fields like 'database', 'comment', and 'owner' to use Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
4. src/datapilot/core/platforms/dbt/schemas/catalog.py:45
- Draft comment:
Field 'value' uses a redundant Optional[Optional[...]] annotation. Use Optional[Union[bool, str, float]] instead. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/datapilot/core/platforms/dbt/schemas/manifest.py:1
- Draft comment:
Switching from Pydantic to dataclasses removes built-in runtime validation. Ensure that data validation/coercion is handled elsewhere if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct about the loss of validation, it's phrased as a request to "ensure" something, which violates our rules. It's asking the author to verify/confirm something rather than pointing out a specific issue that needs fixing. Additionally, if validation was important, it should have been caught in testing or review of the actual implementation. The comment raises a valid technical point about a real architectural change. Maybe removing validation could cause runtime issues that tests wouldn't catch. While the validation concern is valid, the comment doesn't propose a specific fix and instead just asks for verification. Per our rules, we should avoid comments that ask authors to check or verify things. Delete the comment. While it raises a valid technical point, it's phrased as a verification request rather than an actionable change request.
6. src/datapilot/core/platforms/dbt/schemas/manifest.py:50
- Draft comment:
The 'node_color' field in AltimateDocs uses a nested Optional (Optional[Optional[str]]). Simplify to Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
7. src/datapilot/schemas/nodes.py:4
- Draft comment:
Dataclass conversion for ModelNode and SourceNode looks correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/datapilot/core/platforms/dbt/schemas/catalog.py:18
- Draft comment:
The type annotation forinvocation_idis written asOptional[Optional[str]]. This appears redundant—usingOptional[str]should suffice. Please verify if this was intentional. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_gDJUbVmfKSR9W4Ii
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Replace Pydantic BaseModel with Python's native dataclasses across schema files to reduce dependencies and simplify codebase. This includes updating field ordering to comply with dataclass requirements (fields with defaults must come after fields without defaults). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
d852bb0 to
25a6061
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 25a6061 in 2 minutes and 27 seconds. Click for details.
- Reviewed
477lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/insights/schema.py:19
- Draft comment:
Consider using a more specific type (e.g. Dict[str, Any]) for 'metadata' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/datapilot/core/platforms/dbt/schemas/catalog.py:26
- Draft comment:
Field 'database' has redundant Optional wrapping; simplify to Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/datapilot/core/platforms/dbt/schemas/catalog.py:27
- Draft comment:
Field 'comment' has redundant Optional wrapping; simplify to Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
4. src/datapilot/core/platforms/dbt/schemas/manifest.py:51
- Draft comment:
Field 'node_color' in AltimateDocs has redundant Optional wrapping; use Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/datapilot/core/platforms/dbt/schemas/manifest.py:405
- Draft comment:
Optional field 'checksum' in AltimateManifestTestNode is missing a default; consider setting it to None. - Reason this comment was not posted:
Comment was on unchanged code.
6. src/datapilot/core/platforms/dbt/schemas/manifest.py:331
- Draft comment:
Field 'package' in AltimateRefArgs has redundant Optional wrapping; simplify to Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
7. src/datapilot/core/platforms/dbt/schemas/manifest.py:51
- Draft comment:
Typo: 'node_color' is typed as Optional[Optional[str]]. Consider using Optional[str] unless the double Optional is intended. - Reason this comment was not posted:
Comment was on unchanged code.
8. src/datapilot/core/platforms/dbt/schemas/manifest.py:65
- Draft comment:
Typo: 'data_type' is typed as Optional[Optional[str]]. This double Optional might be unintentional; consider using Optional[str]. - Reason this comment was not posted:
Comment was on unchanged code.
9. src/datapilot/core/platforms/dbt/schemas/manifest.py:66
- Draft comment:
Typo: 'quote' is typed as Optional[Optional[bool]]. Consider using Optional[bool] unless the double Optional is deliberate. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_3rncSk1BMvYtXvvy
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| class AltimateCatalogMetadata: | ||
| dbt_schema_version: Optional[str] = "https://schemas.getdbt.com/dbt/catalog/v1.json" | ||
| dbt_version: Optional[str] = "0.19.0" | ||
| generated_at: Optional[datetime] = "2021-02-10T04:42:33.680487Z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field 'generated_at' is typed as datetime but default is a string. Convert the default to a datetime object or update the type.
| generated_at: Optional[datetime] = "2021-02-10T04:42:33.680487Z" | |
| generated_at: Optional[datetime] = datetime.strptime("2021-02-10T04:42:33.680487Z", "%Y-%m-%dT%H:%M:%S.%fZ") |
| dbt_schema_version: Optional[str] = "https://schemas.getdbt.com/dbt/catalog/v1.json" | ||
| dbt_version: Optional[str] = "0.19.0" | ||
| generated_at: Optional[datetime] = "2021-02-10T04:42:33.680487Z" | ||
| invocation_id: Optional[Optional[str]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove redundant Optional wrapping in 'invocation_id'; use Optional[str] instead.
| invocation_id: Optional[Optional[str]] = None | |
| invocation_id: Optional[str] = None |
| name: str | ||
| database: Optional[Optional[str]] = None | ||
| comment: Optional[Optional[str]] = None | ||
| owner: Optional[Optional[str]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field 'owner' has redundant Optional wrapping; simplify to Optional[str].
| metadata: AltimateCatalogTableMetadata | ||
| columns: Dict[str, AltimateCatalogColumnMetadata] | ||
| stats: Dict[str, AltimateCatalogStatsItem] | ||
| unique_id: Optional[Optional[str]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field 'unique_id' has redundant Optional wrapping; simplify to Optional[str].
| unique_id: Optional[Optional[str]] = None | |
| unique_id: Optional[str] = None |
| unique_id: str | ||
| fqn: List[str] | ||
| alias: str | ||
| checksum: Optional[AltimateFileHash] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional field 'checksum' in AltimateManifestNode lacks a default; consider adding '= None'.
| checksum: Optional[AltimateFileHash] | |
| checksum: Optional[AltimateFileHash] = None |
| fqn: List[str] | ||
| alias: str | ||
| checksum: Optional[AltimateFileHash] | ||
| access: Optional[AltimateAccess] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional field 'access' in AltimateManifestNode lacks a default; consider adding '= None'.
| access: Optional[AltimateAccess] | |
| access: Optional[AltimateAccess] = None |
| class AltimateRefArgs: | ||
| name: str | ||
| package: Optional[Optional[str]] = None | ||
| version: Optional[Optional[Union[str, float]]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field 'version' in AltimateRefArgs has redundant Optional wrapping; simplify to Optional[Union[str, float]].
| version: Optional[Optional[Union[str, float]]] = None | |
| version: Optional[Union[str, float]] = None |
Replace Pydantic BaseModel with Python's native dataclasses across schema files to reduce dependencies and simplify codebase. This includes updating field ordering to comply with dataclass requirements (fields with defaults must come after fields without defaults).
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
Important
Refactor to replace Pydantic BaseModel with dataclasses across schema files, adding @DataClass decorators and adjusting field order.
Pydantic BaseModelwithdataclassesinschema.py,catalog.py,manifest.py, andnodes.py.@dataclassdecorator to classes likeInsightResult,AltimateCatalogMetadata,DBTVersion,ModelNode, andSourceNode.This description was created by
for 25a6061. You can customize this summary. It will automatically update as commits are pushed.