Define and export union types #73
Conversation
WalkthroughThe recent updates in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
syou6162
force-pushed
the
feature/add_union_types
branch
from
a5b9e2d
to
d1a87fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (1)
- dbt_artifacts_parser/parser.py (5 hunks)
Additional comments: 1
dbt_artifacts_parser/parser.py (1)
- 82-99: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [74-86]
The
Manifesttype alias is comprehensive, covering versions 1 through 11. This is a good approach for future-proofing and maintainability.
dbt_artifacts_parser/parser.py
Outdated
| # | ||
| # catalog | ||
| # | ||
| def parse_catalog(catalog: dict) -> Union[CatalogV1]: | ||
| Catalog = Union[CatalogV1] |
There was a problem hiding this comment.
Ensure the Catalog type alias is future-proof by considering the inclusion of future versions now or implementing a strategy for easy extension.
| def parse_catalog(catalog: dict) -> Catalog: | ||
| """Parse catalog.json | ||
|
|
||
| Args: | ||
| catalog: dict of catalog.json | ||
|
|
||
| Returns: | ||
| Union[CatalogV1] | ||
| Catalog |
There was a problem hiding this comment.
The function parse_catalog correctly uses the Catalog type alias. However, consider adding error handling for unexpected dbt_schema_version values to enhance robustness.
dbt_artifacts_parser/parser.py
Outdated
| def parse_manifest(manifest: dict) -> Manifest: | ||
| """Parse manifest.json | ||
|
|
||
| Args: | ||
| manifest: A dict of manifest.json | ||
|
|
||
| Returns: | ||
| Union[ManifestV1, ManifestV2, ManifestV3, ManifestV4] | ||
| Manifest |
There was a problem hiding this comment.
The parse_manifest function is well-structured and handles multiple versions. Ensure consistency in error messages for better user experience.
dbt_artifacts_parser/parser.py
Outdated
| RunResults = Union[RunResultsV1, RunResultsV2, RunResultsV3, RunResultsV4, RunResultsV5] | ||
|
|
There was a problem hiding this comment.
The RunResults type alias is correctly defined. Consider documenting the rationale for the chosen versions to aid future maintenance.
| def parse_run_results(run_results: dict) -> RunResults: | ||
| """Parse run-results.json | ||
|
|
||
| Args: | ||
| run_results: A dict of run-results.json | ||
|
|
||
| Returns: | ||
| Union[RunResultsV1, RunResultsV2, RunResultsV3, RunResultsV4]: | ||
| RunResults |
There was a problem hiding this comment.
In parse_run_results, validate the run_results dict structure before parsing to prevent potential runtime errors.
dbt_artifacts_parser/parser.py
Outdated
| Sources = Union[SourcesV1, SourcesV2, SourcesV3] | ||
|
|
There was a problem hiding this comment.
The Sources type alias includes versions 1 through 3. This is appropriate, but document the process for adding new versions.
| def parse_sources(sources: dict) -> Sources: | ||
| """Parse sources.json | ||
|
|
||
| Args: | ||
| sources: A dict of sources.json | ||
|
|
||
| Returns: | ||
| Union[SourcesV1, SourcesV2, SourcesV3] | ||
| Sources |
There was a problem hiding this comment.
parse_sources function should handle unexpected dbt_schema_version values more gracefully to improve error reporting.
|
@syou6162 I'm so sorry for the delay of my reply and thank you for the contribution. As we have supported dbt 1.8 (manifest v12 and run-results v6) are migrating the supported version of pydantic from v1 to v2, we have some conflicts in the pull request. Can you please catch up with the changes? |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- dbt_artifacts_parser/parser.py (6 hunks)
Additional Context Used
Ruff (1)
dbt_artifacts_parser/parser.py (1)
227-227: SyntaxError: Expected a statement
Additional comments not posted (6)
dbt_artifacts_parser/parser.py (6)
17-17: Ensure the new imports are utilized effectively throughout the module.
47-47: The introduction of theCatalogtype alias is a good practice for simplifying type annotations and improving code readability.
50-57: The functionparse_catalogcorrectly uses theCatalogtype alias. Consider adding error handling for unexpecteddbt_schema_versionvalues to enhance robustness.
Line range hint
76-89: TheManifesttype alias is well-defined, covering all current versions. This is a scalable approach that accommodates future versions easily.
92-99: The functionparse_manifestis well-structured and handles multiple versions. Ensure consistency in error messages for better user experience.
311-321: TheSourcestype alias and theparse_sourcesfunction are correctly implemented. Ensure that error handling is robust and consider future versions when raising exceptions.
syou6162
force-pushed
the
feature/add_union_types
branch
from
7a71273
to
5c4d793
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- dbt_artifacts_parser/parser.py (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- dbt_artifacts_parser/parser.py
syou6162
force-pushed
the
feature/add_union_types
branch
from
5c4d793
to
0163006
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- dbt_artifacts_parser/parser.py (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- dbt_artifacts_parser/parser.py
|
Unfortunately, it seems that we can't use the type aliasing in python 3.8 and 3.9. The two are sill supported versions, though I am not sure how many users are impacted by cutting them off. Do you have any idea to solve the issue? |
|
Alright. Let me close this at the moment. We will have other conflicts as supporting newer versions of dbt, since there is a long time until the end-of-life of the version. Please feel free to reopen it, if it gets ready. |
Background
If I simply use dbt-artifacts-parser, that is often enough, since types like
ManifestV1are provided.However, if a third-party tool that supports various versions of dbt artifacts uses dbt-artifacts-parser (e.g., datnguye/dbterd#86 ), then the
ManifestV1,ManifestV2,ManifestV3, ... types withUnion.We did not think it desirable to define these in third-party tools, since every time there is a change in dbt-artifacts-parser, the third-party tool also has to modify the type.
What I Changed
I defined
Uniontypes so that they can be used by third-party libraries.