fix Upward Find for venv #759#2791
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Pyrefly’s virtual environment discovery to search upward through ancestor directories so a project can use a .venv located in a parent directory (addresses #759).
Changes:
- Refactors venv discovery into a per-root helper (
find_in_root) and applies it acrossproject_pathancestors. - Adds unit tests ensuring ancestor search works and prefers the nearest matching ancestor venv.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| pub fn find(project_path: &Path) -> Option<PathBuf> { | ||
| project_path.ancestors().find_map(find_in_root) | ||
| } |
connernilsen
left a comment
connernilsen
left a comment
There was a problem hiding this comment.
Hey @asukaminato0721, thanks for doing this. This has been on the todo list for a while, so it's awesome that you picked it up.
There are a few requests below, but I don't think they should be too difficult to fix. Let me know if you have further questions or want to discuss anything.
| fn search_roots(project_path: &Path) -> impl Iterator<Item = &Path> { | ||
| project_path | ||
| .ancestors() | ||
| .take_while(|path| !path.as_os_str().is_empty()) | ||
| } | ||
|
|
||
| pub fn find(project_path: &Path) -> Option<PathBuf> { | ||
| search_roots(project_path).find_map(find_in_root) |
There was a problem hiding this comment.
Doing an upward filesystem walk to then do a recursive downward search can be very expensive, which is why I originally designed this to be very limited in scope. I have two suggestions for how to improve this, but I'm curious about your/others' thoughts.
- instead of looking at all
project_path.ancestors(), only look at ancestors where a candidate dir exists (something likepath.ancestors().flatten(|a| CANDIDATE_DIRS.iter().map(|c| a.join(c))).filter(|p| p.exists())). This I think would be necessary at minimum. - stop the upward search at the first config file or marker file (
path.ancestors().take_while(|p| !ConfigFile::CONFIG_FILE_NAMES.chain(ConfigFile::ADDITIONAL_ROOT_FILE_NAMES).any(|c| p.join(c).exists())))
Summary
Fixes #759
keeping the existing per-root scan and applying it across project_path plus its ancestors, so Pyrefly now finds the nearest ancestor .venv instead of only searching downward from the current root.
Test Plan
add test