Skip to content
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

Replace 'python.jediEnabled' setting with 'python.languageServer' setting #7010

Closed
erictraut opened this issue Aug 17, 2019 · 22 comments
Closed
Assignees
Labels
area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. feature-request Request for new features or functionality
Milestone

Comments

@erictraut
Copy link

The Python extension currently supports the boolean setting "python.enableJedi". It's true, the Jedi language server is used. If it's false, the Microsoft Python Language Server is used.

This change would replace the "python.enableJedi" setting with "python.languageServer", a string-based setting that initially supports three values: 'jedi', 'microsoft', and 'none'. The first two values map to the old binary settings. The third value results in no language server being loaded. This allows users to install other language servers like pyright without double analysis overhead and duplicate references, type completion suggestions, etc.

All other behaviors (including default behaviors) would remain the same.

Previous versions of the "python.enableJedi" setting should be migrated automatically.

@erictraut erictraut added triage-needed Needs assignment to the proper sub-team feature-request Request for new features or functionality labels Aug 17, 2019
erictraut pushed a commit to erictraut/vscode-python that referenced this issue Aug 18, 2019
…guageServer' setting, supporting 'jedi', 'microsoft', or 'none' values.
@brettcannon brettcannon added this to the 2019 - August Sprint 16 milestone Aug 19, 2019
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Aug 19, 2019
@brettcannon brettcannon added triage-needed Needs assignment to the proper sub-team area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. and removed triage-needed Needs assignment to the proper sub-team labels Aug 19, 2019
@karrtikr karrtikr self-assigned this Aug 19, 2019
karrtikr pushed a commit that referenced this issue Aug 22, 2019
#7011)

* #7010: Replaced 'python.enableJedi' setting with 'python.languageServer' setting, supporting 'jedi', 'microsoft', or 'none' values.

* Fixed lint error.

* Fixed regression that broke smoke tests and performance tests.

* Code review changes:
* Fixed existing spelling of "langauge" to "language"
* Added test cases for replacement of enableJedi setting
* Fixed existing replacement regular expressions which included unescaped periods
karrtikr pushed a commit that referenced this issue Aug 22, 2019
@karrtikr
Copy link

validated

@ghost ghost removed the needs PR label Aug 26, 2019
karrtikr pushed a commit that referenced this issue Aug 27, 2019
@karrtikr karrtikr reopened this Aug 27, 2019
@ghost ghost added the triage-needed Needs assignment to the proper sub-team label Aug 27, 2019
@karrtikr
Copy link

We have an issue with the last PR. It breaks Intellicode. We would need to change the code in Intellicode to look for the new setting and fall back to the old setting (we could base this on version too but that seems more brittle). The existing code that broke is here:

https://devdiv.visualstudio.com/DevDiv/_git/Pythia4Python?path=%2Fvscode-intellicode-python%2Fsrc%2Fvscode-intellicode-python.ts&version=GBmaster&line=14&lineEnd=28&lineStartColumn=1&lineEndColumn=12&lineStyle=plain

karrtikr pushed a commit that referenced this issue Aug 27, 2019
…uageServ…" (#7118)

* Revert "#7010: Replaced 'python.enableJedi' setting with 'python.languageServ… (#7011)"

This reverts commit 70a866c.

* Fix things
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Aug 27, 2019
@karrtikr karrtikr removed their assignment Aug 28, 2019
@karrtikr
Copy link

karrtikr commented Nov 28, 2019

For merging #7010. Work item for spike #8418.

The existing code that broke due to replacing python.jediEnabled setting with python.languageServer is here:
https://devdiv.visualstudio.com/DevDiv/_git/Pythia4Python?path=%2Fvscode-intellicode-python%2Fsrc%2Fvscode-intellicode-python.ts&version=GBmaster&line=14&lineEnd=28&lineStartColumn=1&lineEndColumn=12&lineStyle=plain

We need to make sure all these 4 cases work:

  1. Old version intellicode + Old python extension -> already works
  2. New version intellicode + Old python extension
  3. New version intellicode + New python extension
  4. Old version intellicode + New python extension

Proposed changes

Intellicode

In the old version, we only check jediEnabled setting. In the new version of Intellicode,

  • We would check if user is using the old or the new python extension using the new setting. (Check if flag usingNewLSSetting in package.json of the extension in present or not)
  • If he is using the old one, we would look for the jediEnabled setting
  • If he is using the new one, we would look for the languageServer setting

If new version of intellicode is being used, the above proposed changes work fine for cases 2 & 3.

Python extension

The issue with case 4 is that old versions of IntelliCode shows a prompt asking to set jediEnabled to false to enable IntelliCode, and will set python.jediEnabled if a button is clicked.

But the new version of the extension works only with the languageServer setting, so the setting Intellicode changed has no effect on it. Due to which Intellicode fails.

Solution:

  • Check if we are using the old version of intellicode or the new one.
  • If using the old one,
    • Show a prompt asking user to upgrade Intellicode to new version to keep using it.

Alternate solution:

  • Always keep settings python.jediEnabled and python.languageServer in sync. If intellicode sets python.jediEnabled to false, then detect it in the extension & change python.languageServer to microsoft as well.
    But keeping settings in sync can have its own problems if user manually starts to change the settings. So the prompt approach is the safest one.

    We decided to not go with the alternate solution, where we have both settings. The problem with that solution is that we don't know what setting to prefer when the extension loads.

Note: Also to reduce number cases like 4, where we need to show the prompt, we can release the fix with intellicode first, so most users no longer use the old version of intellicode. Then after a while we can release the insiders fix in the extension.

@uriva
Copy link

uriva commented Nov 28, 2019

I don't know the status of this, but for me having analysis on is a terrible working experience (the pc is terribly sluggish, I've already reported several issues on the LS issue tracker during the last few months but things have largely stayed this way). I still want the benefit of having formatting, so I don't want to disable the entire extension. Can there be a flag to disable analysis altogether or I'm in an all or nothing scenario atm?

@karrtikr
Copy link

karrtikr commented Nov 28, 2019

For now, you can simply use jedi, i.e set python.jediEnabled to true. There is no analysis there.

@uriva
Copy link

uriva commented Nov 28, 2019

That is also slow from time to time...

@karrtikr
Copy link

If you want to turn the entire intellisense off, you can do it as soon as this issue is addressed. Until then there is no alternative but to use Jedi if you do not want any analysis.

@karrtikr
Copy link

karrtikr commented Dec 4, 2019

Work items after the discussion:

  • solidify feature flag setting (design)
  • intellicode PR to use the feature flag
  • replace the jediEnabled setting with languageServer in the extension (can use eric's original PR) and do the migrations
  • update all of our documentation to reflect the setting change

@erictraut
Copy link
Author

@karrtikr, sounds like there's an agreed-upon plan. Do you have an ETA for when this will be published? I continue to get a stream of complaints/requests from pyright users about this, and I'd like to be able to give them a timeline for a fix.

@karrtikr
Copy link

karrtikr commented Dec 4, 2019

@brettcannon @luabud Could you help with a timeline?

@brettcannon
Copy link
Member

@erictraut No timeline other than we hope early in the next quarter (although it's also dependent on IntelliCode making a release which isn't under our control).

@MikhailArkhipov
Copy link

Per offline discussion, taking over it.

@MikhailArkhipov
Copy link

#9182

@ghost ghost removed the needs PR label Dec 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 26, 2019
@ghost ghost added the triage-needed Needs assignment to the proper sub-team label May 19, 2020
@MikhailArkhipov MikhailArkhipov changed the title Replace 'python.enableJedi' setting with 'python.languageServer' setting Replace 'python.jediEnabled' setting with 'python.languageServer' setting May 19, 2020
@karthiknadig karthiknadig added needs PR and removed triage-needed Needs assignment to the proper sub-team labels May 20, 2020
@MikhailArkhipov
Copy link

#11834

@ghost ghost removed the needs PR label Jun 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

9 participants