-
Notifications
You must be signed in to change notification settings - Fork 434
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
[vscode] Use the python extension venv #820
Conversation
df8cc87
to
592a3c3
Compare
@@ -34,7 +34,7 @@ | |||
"cson": "^4.1.0", | |||
"tslint": "^5.8.0", | |||
"tslint-microsoft-contrib": "^5.0.1", | |||
"typescript": "^2.6.1", | |||
"typescript": "^3.7", |
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 think it would be good to also add a lock file. This can make it easier to replicate successful builds. I can do this in a separate PR.
12e1506
to
c132e3a
Compare
Hi @connernilsen, would it be possible to get a review on this? It's pretty awkward to use the pyre vscode extension without this change. |
Hey @vthemelis, thanks for the PR! I'm honestly not too familiar with this part of Pyre. Let me see if I can find someone who is more familiar with it and can provide a good review. |
Thank you very much @connernilsen ! |
8322e8f
to
5892a19
Compare
ac84dea
to
5b70c7f
Compare
Hi guys, can I still get a review for this? |
Sorry, this fell through the cracks last week. Reaching out to people now. |
thank you! 🙏 |
Alright @vthemelis, thanks for making these changes! The following is a followup that one of my teammates requested:
|
Thanks for the review @connernilsen! There is an event listener that restarts pyre when the user switches environments here Did that not work for you? I'm happy to also add a palette item as well. Would that be for restarting the Pyre server in the extensions? |
@kinto0 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Hello @kinto0, thanks for importing the PR! Is there anything I can do to help merge it? |
For test failures on Python < 3.10 see: Would this properly address the situation where I |
Thanks, I can merge
I don't think so. It's not really how I use pyre (I install it in my local environment) but can look into this in a separate PR. |
sorry about the delay. we had some issues authenticating with the vscode store blocking us from deploying new extensions that should be resolved soon. |
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.
would you mind taking a look at my error? I see it when testing with or without the python extension enabled
Thanks a lot for the review @kinto0 ! I pushed a commit to fix the issue. See: #820 (comment) |
@kinto0, does this work for you now? |
did you make a change? I don't see it on my side |
Yes, it's the last commit in the PR. It's also referenced by hash in my reply to your in-line comment. You may need to import the commit in Phabricator. |
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 left this review after e9147b2 but forgot to publish it... my bad.
const activatedEnvPath = pythonExtension.exports.environments.getActiveEnvironmentPath(); | ||
const pyrePath = inferPyreCommand(activatedEnvPath); | ||
|
||
if (pyrePath) { |
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.
if no pyrePath, we should resort to the default one
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 added an extra case in the findPyreCommand
and it now will look in PATH
for pyre to use as a fallback.
@kinto0 , thanks, pushed another commit. |
f271c97
to
c36598d
Compare
* `s/inferPyre/findPyre/g` * Use the default pyre command as a fallback if it exists.
c36598d
to
737154b
Compare
Hi @kinto0 , anything else I can do to help merge this? |
@kinto0 friendly ping :) |
Hi @kinto0 , would it be okay if I had another review at this? Please let me know if you'd rather I forked this. |
For more accurate typing.
Hi @stroxler , would you say it's a better idea for me to fork this VSCode extension? |
I hope you'll do it. I've waited for your update to be merged for almost two months. Great work, but it's too bad it takes ages for someone to respond. |
@xshapira will try to publish something over the weekend. |
Fork created here: https://github.com/vthemelis/pyre-lsp Will add CICD and publish a version soon. |
Published here: https://marketplace.visualstudio.com/items?itemName=vthemelis.pyre-lsp Feel free to open an issue if something's not right. |
Hi @vthemelis sorry I hadn't been following this PR, I'll try to see what's blocking it next week. But regardless, I think given that the Pyre team has not been very responsive sometimes (sorry about that), forking the extension is a very reasonable approach at least for the moment. It's easier in some ways to work in a separate github repo anyway for the language server, because (a) internally we use a different language server that is more aware of our monorepo and buck integration, which means almost all changes will be prompted by open-source users (b) any change to the main pyre-check repo has to go through internal CI which can make it more work to get PRs merged. That's not really a problem for changes to pyre itself since the setups are similar, but for the language servers (which share nothing) it's almost entirely overhead, and as you've seen we're not currently as responsive as we'd like to be |
@kinto0 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
sorry again about all the delays. I was on vacation for a week and then this fell through. I'm merging your code + the extension is now updated to .2. |
Thank you very much for all the help and input @kinto0 ! I hope I didn't disturb your holiday. |
Summary
Fixes #6
Fixes #645
Fixes #183
If the VsCode Python extension is installed, use the Python interpreter that is selected in the extension to start the pyre language server. Also, subscribe to environment switches and restart the pyre language server with the new environments.
If the extension is not present, we retain the original behaviour of the extension.
This is very useful for people that may have multiple environments installed but is more useful for people that use the VsCode SSH extension and cannot benefit from this workaround.
Test Plan
I ran the extension locally and it seems to be picking up the interpreter path.