Skip to content

Conversation

@cswartzvi
Copy link
Contributor

A recent change to dlt in dlt-hub/dlt#2430 moved the loader_file_format parameter from pipeline.normalize to pipeline.extract. This caused CI tests for the dlt plugin to fail (most notably in #1305).

Changes

Updated tests/plugins/test_dlt_extensions.py, moving loader_file_format from pipeline.normalize to pipeline.extract.

How I tested this

Covered by existing tests/plugins/test_dlt_extensions.py.

Notes

N/A

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 everything up to 5e8ad96 in 1 minute and 8 seconds. Click for details.
  • Reviewed 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft 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% <= threshold 50% This comment is asking for confirmation about a change related to the dlt API. 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:
    Pass loader_file_format to pipeline.extract per the updated dlt API (dlt-hub/dlt#2430).
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% 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 the loader_file_format from pipeline.normalize call 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@skrawcz skrawcz merged commit fe481ad into apache:main May 12, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants