Add POETRY_VIRTUALENVS_IN_PROJECT environment variable support#1211
Add POETRY_VIRTUALENVS_IN_PROJECT environment variable support#1211karthiknadig merged 3 commits intomainfrom
Conversation
Co-authored-by: karthiknadig <3840081+karthiknadig@users.noreply.github.com>
karthiknadig
left a comment
There was a problem hiding this comment.
PR Review: Add POETRY_VIRTUALENVS_IN_PROJECT environment variable support
Overall: The core idea is correct and well-motivated — the extension should honor POETRY_VIRTUALENVS_IN_PROJECT to match the PET server's behavior. The isPoetryVirtualenvsInProject() function logic is clean and correct. However, several issues need addressing before merge:
Issues
-
Unrelated
package-lock.jsonchanges — The PR adds"peer": trueto ~15 unrelated packages. This noise should be removed. -
No tests for the actual behavior change — The tests only cover the
isPoetryVirtualenvsInProject()helper. The real feature — how it affects environment classification innativeToPythonEnv— is untested. This is the main risk area. -
Redundant assignment —
isGlobalPoetryEnv = falseon the truthy branch re-assigns the initial value. A guard clause pattern would be cleaner and reduce nesting. -
Testability — Direct
process.envreads make the integration point hard to mock. Consider a small refactor to enable testing thenativeToPythonEnvpath.
… use guard clause, add nativeToPythonEnv integration tests Co-authored-by: karthiknadig <3840081+karthiknadig@users.noreply.github.com>
karthiknadig
left a comment
There was a problem hiding this comment.
Re-review: All previous feedback addressed
The PR now looks much cleaner. All four issues from my previous review have been resolved:
package-lock.jsonnoise removed- Guard clause restructure — no more redundant
isGlobalPoetryEnv = false isPoetryVirtualenvsInProjectaccepts optional parameter — enables testing withoutprocess.envmanipulation for most cases- Integration tests added — 5 tests covering env var + project combinations in
nativeToPythonEnv
Remaining minor observations (non-blocking)
- Hardcoded POSIX paths in tests — works but slightly inconsistent with project conventions
nativeToPythonEnvexport — consider marking as@internal- No positive
POETRY_GLOBALtest — all integration tests verifygroup === undefined; a test confirminggroup === POETRY_GLOBALfor envs in the global directory would round out coverage
Overall the logic is correct, the code is clean, and the test coverage is reasonable. Approving.
package-lock.jsonchangesisPoetryVirtualenvsInProject()accept optional parameter for testabilitynativeToPythonEnvfor testabilitynativeToPythonEnvenv var classification behavior (5 tests)process.envfallback test forisPoetryVirtualenvsInProjectOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.