Skip to content

Fix a regression in recursive dependency enumeration #693

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented May 6, 2025

There were two bugs in a previous change to recursive dependency enumeration. The first relates specifically to defaultdict behaviors, and the other to lambda capture.

Fixes #646

Keeping in draft until I can add tests.

CI is blocked on colcon/ci#40

There were two bugs in a previous change to recursive dependency
enumeration. The first relates specifically to defaultdict behaviors,
and the other to lambda capture.

Fixes 520052d
Copy link
Member

@christophebedard christophebedard 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, but tests would definitely be good!

Comment on lines +117 to +118
non_map_categories = recursive_categories
recursive_categories = defaultdict(lambda: non_map_categories)
Copy link
Member

Choose a reason for hiding this comment

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

Reading up on lambda captures, you might be able to do this (pardon the line length):

Suggested change
non_map_categories = recursive_categories
recursive_categories = defaultdict(lambda: non_map_categories)
recursive_categories = defaultdict(lambda recursive_categories=recursive_categories: recursive_categories)

but it's not really better than the current solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants