Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Fixes minor bug with default arguments #281

Merged
merged 1 commit into from
Jan 23, 2023
Merged

Fixes minor bug with default arguments #281

merged 1 commit into from
Jan 23, 2023

Conversation

skrawcz
Copy link
Collaborator

@skrawcz skrawcz commented Jan 23, 2023 •
edited
Loading

Fixes erroneous error.

Changes

  • adds check for required to only look at nodes required for execution

How I tested this

  • locally via example code I'm working on
  • unit test

Notes

  • better unit testing earlier would have caught this

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.

@skrawcz
Fixes minor bug with default arguments
19f452e 
There was a bug where we would consider nodes that
were not part of the execution graph to see if all the inputs
to a node were acceptable.

E.g. if two functions required the same input, but one had default arguments,
and the function without a default was not on the execution path, we'd error.
Why? Because we'd naively look at all the node's dependents to see if one was
required, rather than restricting to the set of nodes required for execution.

Adds to unit test; aside some of our driver code could be made easier to unit test...
@skrawcz skrawcz merged commit 9bc63e0 into main Jan 23, 2023
@skrawcz skrawcz deleted the fix_default_arg_check branch January 23, 2023 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@skrawcz