fix: move the snapshot branch into get_depends - #143
Merged
Conversation
RoboStack#134 added the snapshot shortcut to _get_direct_depends, but it reads ignore_pkgs and cache_key, which only exist in the caller. ruff fails on master with two F821 errors and the code raises NameError for any snapshot distro. The block was written for get_depends: it computes a recursive closure and caches under the recursive cache key. Move it there. Also fix the fixtures that construct Distro via __new__ without the attributes the code reads, and mock the walker method _get_direct_depends actually calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI is red on
masterright now — all threetestjobs fail on e10a578, so every open PR is red through no fault of its own.pixi run lint-checkis what fails:#134 added a snapshot shortcut to
_get_direct_depends, but the block readsignore_pkgsandcache_key, and neither exists in that scope. They're locals ofget_depends, which is where the block was clearly written for: it computes a full recursive closure via_get_snapshot_recursive_dependsand stores it underself._depends_cache[cache_key], the recursive cache._get_direct_dependsreturns direct dependencies and caches them per package in_direct_depends_cache.So this isn't only a lint problem. Any snapshot-based distro would hit
NameErroras soon as it resolved a dependency, and ros-rolling runs with a snapshot. Moving the block intoget_dependsputs it back where its variables live and where the semantics match.There were also 4 failing tests, all fixture problems that the lint failure was masking:
test_distro_dependencies.pybuildsDistrothroughDistro.__new__and never setssnapshot, so the walk raisedAttributeErroras soon as it reached the snapshot check.make_snapshot_distroandtest_empty_snapshot_keeps_live_rosdistro_behaviorset_depends_cachebut not_direct_depends_cache.test_empty_snapshot_keeps_live_rosdistro_behaviorstubbed_walker.get_recursive_depends, but_get_direct_dependscalls_walker.get_dependsonce per dependency type. The stub was never reached, so the mock leaked through asTypeError: 'Mock' object is not iterable. It now stubs the method the code actually calls, matching howtest_distro_dependencies.pydoes it.Testing
pixi run testgoes from 4 failed / 146 passed to 150 passed, andpixi run lint-checkpasses.I have not run the generator against a real snapshot distro, so I have not confirmed end to end that the snapshot dependency resolution produces the right packages — only that it no longer blows up and that the tests from #134 pass. Someone closer to that feature should sanity-check the intent, since I fixed this from the outside; it's possible the shortcut was meant to be a direct-dependency lookup feeding the existing walk instead, which would be a bigger change.