pyre-check
pyre-check copied to clipboard
facebook
[vscode] Use the python extension venv
Summary
Fixes https://github.com/facebook/pyre-check/issues/6, https://github.com/facebook/pyre-check/issues/645, and https://github.com/facebook/pyre-check/issues/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.
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.
Sorry, this fell through the cracks last week. Reaching out to people now.
Alright @vthemelis, thanks for making these changes! The following is a followup that one of my teammates requested:
Would you be able to add a command palette item for allowing changes to this without having to restart VSCode? It looks like the changes here would only work for the initial startup workflow.
Thanks for the review @connernilsen!
There is an event listener that restarts pyre when the user switches environments here
https://github.com/facebook/pyre-check/pull/820/files#diff-1141a7840adc35ecfd0234d6fe8484e7aec33f72306fd32bada0037336ea1f3eR47-R56
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:
- #832
Would this properly address the situation where I pipx install pyre-check ?
For test failures on Python < 3.10 see:
Thanks, I can merge main into this branch once you merge this PR.
Would this properly address the situation where I
pipx install pyre-check?
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.
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: https://github.com/facebook/pyre-check/pull/820#discussion_r1583433435
@kinto0, does this work for you now?
did you make a change? I don't see it on my side
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.
Hi @kinto0 , would it be okay if I had another review at this? Please let me know if you'd rather I forked this.
Hi @stroxler , would you say it's a better idea for me to fork this VSCode extension?
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.
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.