- 
                Notifications
    You must be signed in to change notification settings 
- Fork 73
Switch to directly invoke python-environments-ext api #849
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors Python environment handling to support both the legacy Python extension API and a new Python Environments extension API. The changes enable runtime selection between the two APIs based on a configuration setting and extension availability.
Key Changes:
- Introduced a new envExtApi.tsmodule containing the complete Python Environments extension API type definitions
- Refactored python.tsto conditionally use either the legacy or new environment API viauseEnvExtension()flag
- Extracted legacy API implementation into legacyPython.tsfor cleaner separation
- Updated test suites to accommodate both API paths with new helper utilities
- Modified debug adapter factory and configuration resolvers to work with the new PythonEnvironmenttype
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description | 
|---|---|
| src/extension/envExtApi.ts | Added comprehensive API type definitions for the new Python Environments extension (1289 lines) | 
| src/extension/common/python.ts | Refactored to support dual API paths with conditional logic based on useEnvExtension() | 
| src/extension/common/legacyPython.ts | Extracted legacy Python extension API implementation from python.ts | 
| src/extension/common/utilities.ts | Added useEnvExtension()function to determine which API to use at runtime | 
| src/extension/debugger/adapter/factory.ts | Updated to use new PythonEnvironmenttype and added version parsing logic | 
| src/extension/debugger/configuration/resolvers/base.ts | Enhanced to resolve interpreter from program file when appropriate | 
| src/extension/debugger/configuration/debugConfigurationService.ts | Added interpreter resolution after variable substitution | 
| src/extension/common/application/commands/reportIssueCommand.ts | Updated to handle both environment API return types | 
| src/test/unittest/common/pythonTrue.unit.test.ts | New test suite for new environment extension path | 
| src/test/unittest/common/pythonFalse.unit.test.ts | New test suite for legacy extension path | 
| src/test/unittest/common/helpers.ts | Added buildPythonEnvironment()test helper function | 
| src/test/unittest/adapter/factory.unit.test.ts | Updated tests to use new PythonEnvironmenttype | 
| src/test/unittest/configuration/resolvers/launch.unit.test.ts | Updated to handle both API return types | 
| src/test/unittest/common/application/commands/reportIssueCommand.unit.test.ts | Updated mock interpreter structure | 
Comments suppressed due to low confidence (1)
src/test/unittest/common/pythonFalse.unit.test.ts:4
- This expression has no effect.
('use strict');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interpreter = interpreterPath ? await resolveEnvironment(interpreterPath.environmentPath.fsPath) : undefined; | ||
| } else if (interpreterPath && 'path' in interpreterPath) { | ||
| interpreter = interpreterPath ? await resolveEnvironment(interpreterPath.path) : undefined; | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
The condition interpreterPath ? on lines 29 and 31 is redundant since it's already checked in the outer if conditions. Simplify to interpreter = await resolveEnvironment(interpreterPath.environmentPath.fsPath); and interpreter = await resolveEnvironment(interpreterPath.path); respectively.
| interpreter = interpreterPath ? await resolveEnvironment(interpreterPath.environmentPath.fsPath) : undefined; | |
| } else if (interpreterPath && 'path' in interpreterPath) { | |
| interpreter = interpreterPath ? await resolveEnvironment(interpreterPath.path) : undefined; | |
| interpreter = await resolveEnvironment(interpreterPath.environmentPath.fsPath); | |
| } else if (interpreterPath && 'path' in interpreterPath) { | |
| interpreter = await resolveEnvironment(interpreterPath.path); | 
| const parseMajorMinor = (v: string) => { | ||
| const m = v.match(/^(\d+)(?:\.(\d+))?/); | ||
| return { major: m ? Number(m[1]) : 0, minor: m && m[2] ? Number(m[2]) : 0 }; | ||
| }; | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
The version parsing logic has a potential issue: if the regex match fails (m is null), accessing m[1] will throw an error. The condition should be m && m[1] ? Number(m[1]) : 0 to safely handle null matches.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|  | ||
| ('use strict'); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
The 'use strict' directive is wrapped in parentheses, making it a no-op expression statement rather than an actual directive. Remove the parentheses: 'use strict';
| ('use strict'); | |
| 'use strict'; | 
| activateEnvsExtension(); | ||
|  | ||
| async function activateExtensions() { | ||
| traceWarn('Value during activateExtensions of useEnvExtension(): ', useEnvExtension()); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
[nitpick] This appears to be debug logging left in the code. Consider removing or converting to traceLog for production code.
| traceWarn('Value during activateExtensions of useEnvExtension(): ', useEnvExtension()); | |
| traceLog('Value during activateExtensions of useEnvExtension(): ', useEnvExtension()); | 
| if (api) { | ||
| disposables.push( | ||
| api.onDidChangeEnvironments(async () => { | ||
| // not sure if this is the right event.... | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
This comment indicates uncertainty about the correctness of the implementation. Either verify that onDidChangeEnvironments is the correct event to use or document why it was chosen. Unresolved uncertainty should not remain in production code.
| // not sure if this is the right event.... | |
| // The Python Environments extension exposes `onDidChangeEnvironments` as the event to signal environment changes. | |
| // See: https://github.com/microsoft/vscode-python/blob/main/src/client/pythonEnvironments/info/api.ts | 
| resource?: Resource, | ||
| ): Promise<PythonEnvironment | EnvironmentPath | undefined> { | ||
| // if I add environmentPath. or there needs to be some conversion between the two here | ||
| //TODO: fix this return type?? | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
This TODO comment suggests an unresolved type design issue. The return type Promise<PythonEnvironment | EnvironmentPath | undefined> should be clarified or fixed before merging.
switch from using the Python extension apis to get the interpreter to instead using the Python Environments extension APIs (when available) as this will be the new best practice