Fix dlt plugin with changes to loader_file_format#1306
Merged
skrawcz merged 1 commit intoapache:mainfrom May 12, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5e8ad96 in 1 minute and 8 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft 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. hamilton/plugins/dlt_extensions.py:59
- Draft comment:
Moved loader_file_format to pipeline.extract as per recent dlt changes. Consider adding an inline comment referencing dlt-hub/dlt#2430 for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about a real code change in the diff. It suggests documenting why the parameter moved, which could be helpful for future maintenance. However, linking to external PRs in comments can become stale and the comment itself may not be essential for understanding the code. The code works correctly either way - the location of the parameter is an implementation detail. The suggested comment could become outdated if the referenced PR link becomes invalid. Also, the parameter move is straightforward enough that it may not need explanation. While the link could become stale, having context about why the parameter moved could save future developers time when working with the dlt integration. The comment suggests a nice-to-have documentation addition but isn't strictly necessary for code correctness or maintenance. The code works fine without the suggested inline comment.
2. hamilton/plugins/dlt_extensions.py:60
- Draft comment:
Removed loader_file_format from normalize call. Please confirm that the new dlt API requires this parameter only in extract. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking for confirmation about a change related to thedltAPI. It is not making a specific code suggestion or pointing out a potential issue. It violates the rule against asking the PR author to confirm their intention or to ensure the behavior is intended.
3. hamilton/plugins/dlt_extensions.py:59
- Draft comment:
Passloader_file_formattopipeline.extractper the updated dlt API (dlt-hub/dlt#2430). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, indicating a change in the API usage. It does not provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
4. hamilton/plugins/dlt_extensions.py:60
- Draft comment:
Remove passing theloader_file_formatfrompipeline.normalizecall since it is no longer supported. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_odn0BdPiuRl2xhVL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
skrawcz
approved these changes
May 12, 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.
A recent change to
dltin dlt-hub/dlt#2430 moved theloader_file_formatparameter frompipeline.normalizetopipeline.extract. This caused CI tests for thedltplugin to fail (most notably in #1305).Changes
Updated
tests/plugins/test_dlt_extensions.py, movingloader_file_formatfrompipeline.normalizetopipeline.extract.How I tested this
Covered by existing
tests/plugins/test_dlt_extensions.py.Notes
N/A
Checklist