Closed ghost closed 3 years ago
Thanks for your contribution! I'm okay with the changes overall. However, there are some issues when I test your branch on my end.
As you can see, the stdout redirection does not work for some reason, and the string formatting doesn't either.
Also you mention
Don't show the Matlab terminal until it's reached the starting message.
But on my side it is still displayed as soon as the command is launched. So overall, I like the consistency changes, but the new features/fixes don't work for me.
Kurwa MATLAB Engine Oh crap, I know what's happening. MATLAB implicitly print
s all unsuppressed variables.
I'll look into hiding the terminal from the user. Fwiw from vscode-python
this seems broken in the API itself..
Python doesn't have the issue, I'll take a look at their repo and see how they booted the terminal..
To be honest, I had a look at not showing the terminal directly before, but I think the only way to get this level of control is to implement a pseudo-terminal. However, this implies redoing a lot of the code. I actually tried it because I thought it would allow me to solve other issues, but I had trouble making it feature-equal with what exists currently, and did not solve all of the problems I expected to solve as easily as I thought.
@apommel I solved the terminal issue in f9013e8. Turns out vscode-python uses createTerminal
without passing text. Switching from Terminal.sendText
to the TerminalOptions
keys fixed it!
You did quite a change in the way the Python executable is looked for, and I am not sure what everything is supposed to do. However, when leaving the pythonPath
option blank, this ends up looking for an executable called undefinedpython.exe
and obviously fails.
It's definitely working on my end. The extension looks for Python 3.8, 3.7 or 2.7 inside the PATH
environment variable. If it's not available in the PATH variable, it will suddenly and aggressively break.
Yeah I get what you are doing. It does not work on my end because I installed it through miniconda, so it is not inside a Python38, Python37 or Python27 folder.
So you may wish to add miniconda2|miniconda3|anaconda2|anaconda3
there but it kinda breaks the point of version checking here, and there may be other Python distributions which won't be covered.
Maybe this is a case of overengineering on my part. A simple python
should work, then the README gets updated to clarify that the 3.7 or 3.8 Python installation must be the first Python executable available in the PATH.
E.g. my PATH looks like this:
C:\Program Files\Python38\Scripts\;C:\Program Files\Python38\;C:\Program Files\Python39\Scripts\;C:\Program Files\Python39\;C:\Program Files (x86)\Common Files\Oracle\Java\javapath;C:\Windows\System32;...
Yeah otherwise it is possible to use the pythonPath
option, which is why I implemented it in the first place. The good thing about your way is that it would have been possible to catch if Python was missing from PATH
, but I think it's enough to catch it when trying to run check_dependencies.py
.
Right that's our work done! Thanks for your patience and all the reviews
PRs for upgrading TSLint and adding a terminal profile coming soon™️
Right, I checked one last time and everything seems fine, I'll merge it then. Thanks again!