refactor: update manifest wrapper to use model_dump and supported lan…#61
Merged
refactor: update manifest wrapper to use model_dump and supported lan…#61
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to f9bde24 in 1 minute and 18 seconds. Click for details.
- Reviewed
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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/platforms/dbt/wrappers/manifest/v12/wrapper.py:91
- Draft comment:
Good use of model_dump() over dict; ensure node.config supports model_dump() for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:186
- Draft comment:
Check behavior for supported_languages: returning None on empty list may be unintended; consider returning an empty list. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a style preference rather than a clear bug. Both None and [] are valid ways to represent "no supported languages". Without seeing the schema or how this field is used, we can't definitively say one is better. The current code is consistent with how other optional lists are handled in this file (see arguments, docs handling). The comment is speculative about the intention. The comment could be right - an empty list might be more semantically accurate than None for representing "no supported languages". Empty collections vs None is a common design discussion point. While the suggestion isn't wrong, the current code follows an established pattern in the codebase. Without clear evidence of problems, changing it could reduce consistency. Delete the comment. This is a style preference without clear evidence of being problematic, and the current code follows consistent patterns in the codebase.
3. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:35
- Draft comment:
Import of AltimateSupportedLanguage added; confirm this is the intended type for language conversion. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_UEM7H2JRd4JU7f7u
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
suryaiyer95
reviewed
Jun 19, 2025
mdesmet
commented
Jun 19, 2025
suryaiyer95
approved these changes
Jun 19, 2025
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
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.
…guages
Important
Refactor
ManifestV12Wrapperto usemodel_dump()for configuration and language handling inwrapper.py.ManifestV12Wrapperinwrapper.pyto usemodel_dump()forAltimateNodeConfigin_get_node().model_dump()forAltimateMacroArgumentin_get_macro().model_dump()forAltimateSourceConfigin_get_source().supported_languagesin_get_macro()usingAltimateSupportedLanguagefor better language handling.This description was created by
for f9bde24. You can customize this summary. It will automatically update as commits are pushed.