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

Allow setting default arguments for dependencies #23

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

JamesWrigley
Copy link
Member

(cherry-picked from one of my giant branches)

@JamesWrigley JamesWrigley self-assigned this Jul 5, 2023
@JamesWrigley JamesWrigley added the enhancement New feature or request label Jul 7, 2023
for name in ctx_file.ordered_vars():
var = ctx_file.vars[name]

try:
# Add all variable dependencies
kwargs = { arg_name: res.get(dep_name)
kwargs = { arg_name: get_dep_or_default(var, arg_name, dep_name)
for arg_name, dep_name in var.arg_dependencies().items() }

# If any are None, skip this variable since we're missing a dependency
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# If any are None, skip this variable since we're missing a dependency
# Check for missing dependencies with no default value in the function

Since we're no longer looking for None. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, indeed :) Fixed in 6085018.

@takluyver
Copy link
Member

Other than that comment fix, LGTM

@JamesWrigley JamesWrigley merged commit 5120206 into master Jul 7, 2023
1 check passed
@JamesWrigley JamesWrigley deleted the dependencies branch July 7, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants