-
-
Notifications
You must be signed in to change notification settings - Fork 636
fix(venv): include pth files at the root of the site-packages folder #3340
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
Summary of ChangesHello @aignas, 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 resolves an issue where Python 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 an issue where .pth
files at the root of site-packages
were not correctly included in virtual environments. The fix involves updating the file extension list and modifying how symlink entries are created for root-level files. The changes in the implementation look correct and address the problem. However, the new test case added to verify this fix is flawed, as it asserts an incorrect behavior and doesn't test the new code path. I've provided a detailed comment with a suggested replacement for the test to ensure the fix is properly validated.
tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl
Outdated
Show resolved
Hide resolved
Before this PR we would not include `pth` files at the root of the `site-packages` folder, but we would include if they are further down the tree. Fixes bazel-contrib#3339 Fixes bazel-contrib#2071
ce0bb1b
to
2f34140
Compare
/gemini review |
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 correctly addresses an issue where .pth
files at the root of the site-packages
folder were not being included in the virtual environment. The fix involves recognizing .pth
as a valid Python file extension and adjusting the symlink creation logic to correctly handle these root-level files. The accompanying test changes effectively validate this fix. I have one suggestion to improve the maintainability of the test code.
tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl
Show resolved
Hide resolved
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.
A functional test would be good, too, but I think its fine for that to be in another PR
As requested in bazel-contrib#3340, this adds a functional test for it all. Tests for bazel-contrib#3339
Before this PR we would not include
pth
files at the root of thesite-packages
folder, but we would include if they are further downthe tree.
Fixes #3339
Fixes #2071