perf/tests: use more appropriate data type for package filter policy packages and add tests#10907
Merged
radoering merged 1 commit intoMay 17, 2026
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
packagesand_package_setmust stay in sync, consider makingpackagesimmutable (e.g., a tuple) or otherwise preventing external mutation, so bugs aren’t introduced by code that modifiespackageswithout updating_package_set. - If the policy is never mutated after initialization, using a
frozensetinstead ofsetfor_package_setcould better express immutability intent and slightly reduce the risk of accidental modification.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `packages` and `_package_set` must stay in sync, consider making `packages` immutable (e.g., a tuple) or otherwise preventing external mutation, so bugs aren’t introduced by code that modifies `packages` without updating `_package_set`.
- If the policy is never mutated after initialization, using a `frozenset` instead of `set` for `_package_set` could better express immutability intent and slightly reduce the risk of accidental modification.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
4a9eca6 to
a0a8f85
Compare
Contributor
Author
|
Addressed Sourcery feedback in a0a8f85 by storing normalized packages as an immutable tuple and using a frozenset for repeated membership checks. |
dimbleby
reviewed
May 17, 2026
a0a8f85 to
2b2a547
Compare
Member
I doubt that real numbers become that high. I assume that 100 is quite much and most of the times it will be less than 10. Nevertheless, frozenset feels like the more appropriate data type. |
radoering
approved these changes
May 17, 2026
2b2a547 to
8e09ed5
Compare
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
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.
PackageFilterPolicynormalizesinstaller.no-binary,installer.only-binary, and related package-filter settings once, butallows()andhas_exact_package()previously scanned the normalized sequence each time they were called.Chooser policy checks run for every candidate link, so this stores the normalized package names as an immutable
frozensetfor repeated membership checks.A small local benchmark over a 1,001-entry normalized package policy:
Pull Request Check List
Local checks: