-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implement copying python import path from opened file #25026
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?
Implement copying python import path from opened file #25026
Conversation
@microsoft-github-policy-service agree |
} | ||
|
||
/** | ||
* Resolves a Python import-style dotted path from an absolute file path. |
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.
As described here, the strategy for resolving the Python import path is as follows:
- If the file is located under any entry in
sys.path
, the path relative to that entry is used. - If the file is located under the current workspace folder, the path relative to the workspace root is used.
- Otherwise, the import path falls back to the file name (without extension).
I believe this approach is reasonable, but if you have any suggestions for improvement, I’d love to hear them!
@@ -1126,6 +1131,11 @@ | |||
} | |||
], | |||
"keybindings": [ | |||
{ | |||
"command": "python.copyImportPath", | |||
"key": "ctrl+alt+shift+i", |
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.
I've set Ctrl+Alt+Shift+I
as the default keybinding.
If you find it inconvenient or have a better suggestion, please let me know!
Hi, just a quick note regarding the CI failure: It appears that the error is not related to the changes in this PR. Please let me know if anything needs to be changed on my end. Thanks! |
@s-kai273 Thanks for creating this PR. Can you create an issue for this, with steps for testing it, and to get community input on this? |
* @param absPath - The absolute path to a `.py` file. | ||
* @returns The resolved import path in dotted notation (e.g., 'pkg.module'). | ||
*/ | ||
private resolveImportPath(absPath: string, pythonPath?: string): string { |
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.
You might look at the import logic as described here:
https://github.com/microsoft/pyright/blob/5d79dbbbcd0e683b07b4bd4a7f1910a835564249/packages/pyright-internal/src/analyzer/importResolver.ts#L1568
That's what Pylance uses to resolve an import. Maybe it's too complicated for this, but it would be cool if the python extension had the same import path as the language server.
Or instead of doing it this way, we add a new api to the pylance extension for the python extension to call.
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.
Thank you for your feedback.
I agree that it would be better to adopt the same implementation as Pylance.
However, I’m wondering whether adding an API specifically for this feature and using it in this context is appropriate.
As I understand it, an LSP server like Pylance is mainly intended to provide language features such as completion, diagnostics, and navigation, so this feature might be outside the typical scope of a language server.
If you think this approach is appropriate, I’d be happy to proceed with it.
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.
Pylance would likely add this command itself if this was used enough. In fact if Pylance was open source, I'd say it would be better if you just added it to Pylance. That way the command would be consistent with other things that happen in the editor.
Whether or not it's worth the effort to add an API to Pylance, I'd say probably not now. We'd likely want to wait for telemetry on how often this command is used and any issues that people entered where the path is wrong.
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.
I looked into the _resolveBestAbsoluteImport
implementation in Pyright that you shared, and I found that the logic is quite extensive.
As you mentioned, it would definitely be great if the Python extension could reuse the same logic as Pyright. Honestly, I’d love to take on that challenge—but maintaining a custom implementation at that scale would likely be too costly.
Instead, I think a more maintainable approach would be to install Pyright as an internal dependency and call the relevant methods directly. However, considering that this would introduce a dependency that overlaps with functionality already provided by Pylance, it might not be ideal to add Pyright internally just for this feature.
As a practical compromise for now, I’m considering refining the current import resolution logic and adding telemetry to better understand usage and potential improvements in the future.
I'd really appreciate your feedback on this direction.
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.
Yeah I like this idea:
As a practical compromise for now, I’m considering refining the current import resolution logic and adding telemetry to better understand usage and potential improvements in the future.
I don't think this command has to be perfect from the beginning. It can be close enough and then if it gets a lot of usage we can come back and figure out how to match the import logic later.
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.
Thanks for the feedback!
I’ll go ahead with that approach.
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.
@@ -0,0 +1,13 @@ | |||
import { execFileSync } from 'child_process'; | |||
|
|||
export function getSysPath(pythonCmd = 'python3'): string[] { |
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 isn't safe if there's a json.py file in the user's workspace (people sometimes open folders without checking). You might want to copy this code here:
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.
Thank you for your insightful comment.
I learned a lot from it.
If we adopt the approach used in Pylance, this method will no longer be necessary.
After this discussion, I’ll address this issue.
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.
Fix it on 1e33952
@karthiknadig I added an instruction of this feature here. |
…273/vscode-python into feature/implement_copy_import_path
…273/vscode-python into feature/implement_copy_import_path
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.
LGTM
Summary
This PR implements a new feature that allows users to copy the Python import path of the currently opened file to the clipboard via the file tab context menu.
What was done
python.copyImportPath
command.Example
For a file at /home/user/project/src/pkg/module.py and sys.path containing /home/user/project/src, the copied import path will be: