Skip to content
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

Fix keep_dot propagation in Driver display functions #1202

Conversation

ChronoJon
Copy link
Contributor

When investigating #1200 I noticed, that keep_dot parameters weren't propagated to the render function.

Changes

The previous non-functional parameters are now passed on.

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.

These are not applicable

  • 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.

👍 Looks good to me! Reviewed everything up to 74c25ca in 8 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. hamilton/driver.py:1200
  • Draft comment:
    The addition of keep_dot=keep_dot ensures that the keep_dot parameter is correctly propagated to the display method, aligning with the intended functionality. This change is necessary for the parameter to have an effect.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR correctly adds the missing keep_dot parameter to the display_downstream_of and display_upstream_of methods. This ensures that the parameter is propagated to the display method of the graph, which is necessary for the intended functionality. The change is straightforward and aligns with the existing pattern in the codebase.
2. hamilton/driver.py:1264
  • Draft comment:
    The display_downstream_of and display_upstream_of methods have similar logic for rendering the graph. Consider refactoring to avoid repetition and adhere to the DRY principle.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The display_downstream_of and display_upstream_of methods have similar logic for rendering the graph, which could be refactored to avoid repetition.

Workflow ID: wflow_9a1kMAF3vC6YBPNU


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the catch!

@skrawcz skrawcz merged commit 083dcce into DAGWorks-Inc:main Oct 23, 2024
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