-
-
Notifications
You must be signed in to change notification settings - Fork 631
fix(venv): group venv prefixes by path component, not raw path #3333
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
Conversation
When files overlap between packages, and dist-info directories are present, it results in a prefix list like `foo foo-bar foo/bar`. When sorted as raw strings, hyphen sorts before slash, so the continuity of path prefixes is violated and they are grouped separately. An error then occurs because both `foo/` and `foo/bar` are created, but the latter is a sub-path of the former. To fix, change the sort key to a tuple of path components. This makes `foo foo-bar foo/bar` sort as `(foo,) (foo, bar), (foo-bar, )`, resulting in the correct order. Fixes bazel-contrib#3204
Summary of ChangesHello @rickeylev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the virtual environment path grouping logic that caused incorrect ordering of paths, especially when Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes a bug in how virtual environment path entries are grouped by changing the sorting key. Instead of sorting raw paths as strings, it now sorts them based on their path components. This correctly groups paths with common prefixes, like foo
and foo/bar
, before other paths like foo-bar
, preventing errors when creating venv symlinks. The change is logical and is accompanied by a new test case that validates the fix. My only suggestion is to improve the comment to make the sorting logic clearer for future maintenance.
When files overlap between packages, and dist-info directories are present, it results in a prefix list like
foo foo-bar foo/bar
. When sorted as raw strings, hyphen sorts before slash, so the continuity of path prefixes is violated and they are grouped separately. An error then occurs because bothfoo/
andfoo/bar
are created, but the latter is a sub-path of the former.To fix, change the sort key to a tuple of path components. This makes
foo foo-bar foo/bar
sort as(foo,) (foo, bar), (foo-bar, )
, resulting in the correct order.Fixes #3204