Leverage ms-python.python extension to resolve python interpreter path automatically #39
Conversation
|
Hi @afri-bit, |
|
sorry for the delayed reply, I was on vacation, I don't usually touch my computer during my vacation :) Thanks for the pull request, I will take a look and come back to you :) |
|
Hey @afri-bit, |
|
hi @hackenbergstefan, |
| @@ -108,17 +108,28 @@ export class SettingsPropertyManager { | |||
| profileObject = general.plainObjectToClass(ConanProfileConfiguration, selectedProfileObject); | |||
| } | |||
|
|
|||
| let pythonInterpreter = await python.getCurrentPythonInterpreter(); | |||
| if (pythonInterpreter !== undefined) { | |||
There was a problem hiding this comment.
Need an if statement before entering this section, since the profileObject might be undefined, it leads to error in case of the undefined object, when you want to set the value to its attribute.
There was a problem hiding this comment.
I tink profileObject?. already checks for undefined or none. But I'm not a typescript expert. At least the description explains like that: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#optional-chaining
There was a problem hiding this comment.
I was expecting the same output as in the explanation but apparently the it didn't do the things as expected.
stack trace: TypeError: Cannot set properties of undefined (setting 'conanPythonInterpreter')
I would suggest to remove the ! and ? assertion from the profileObject and surround the whole code with simple if condition before entering the section of entering the python interpreter, see code below. With this we can make sure that it will return undefined in case the object doesn't exist
.
.
if(profileObject) {
let pythonInterpreter = await python.getCurrentPythonInterpreter();
if (pythonInterpreter !== undefined) {
...
}
}
return profileObjectThere was a problem hiding this comment.
I was not smart enough :P The if (! was the problem. profileObject? evaluates to false so if (!profileObject?...) is true 🤦♂️
| profileObject!.conanPythonInterpreter = pythonInterpreter; | ||
| } | ||
| if (!profileObject?.conanExecutable) { | ||
| profileObject!.conanExecutable = path.join(path.dirname(pythonInterpreter), "conan"); |
There was a problem hiding this comment.
This might work in case of virtual environment, in case of system wide python, the path where conan executable is located, is completely in different place.
I would suggest to check if the conan binary exist after building the path, otherwise set to empty string as before.
| } | ||
| } | ||
|
|
||
| console.log(profileObject); |
There was a problem hiding this comment.
Remove this line before merging later on :)
| let pythonInterpreter = await python.getCurrentPythonInterpreter(); | ||
| if (pythonInterpreter !== undefined) { | ||
| if (!profileObject?.conanPythonInterpreter) { | ||
| profileObject!.conanPythonInterpreter = pythonInterpreter; |
There was a problem hiding this comment.
As far as i understood, is that this line of code will be used, in case the user doesn't set the python interpreter inside the configuration, so the extension will be smart enough to get the python interpreter from the python extension, really nice
But we must provide the visibility of this information to the user, from where the python interpreter is used during the usage of the extension, otherwise, user might be confused due to hidden information.
There was a problem hiding this comment.
What do you suggest? Mentioning in the README and printing the value of pythonInterpreter on the extension's output.
There was a problem hiding this comment.
READMEis definitely the first step- The extension output is also good to show it there (I see you already did that 😃)
I am thinking to put the information maybe in the tooltip information from the toolbar where the selected profile is shown. But if the two bullet points from above are there, i think we can move on.
I will take care of the rest, since it is more or less nice to have before the user gets confused, where the extension takes the python interpreter from 😄
There was a problem hiding this comment.
I like the tooltip idea! :-)
I've implemented.
e35fa9b to
7f49fe9
Compare
|
Thanks for merging! 😊 |
If the
ms-python.pythonextension is installed (which is true for most developers, I guess) VSConan can automatically resolve the Python interpreter path and Conan executable using the currently activated Python Environment.