Skip to content

Conversation

jakebailey
Copy link
Member

For #11083

The PR for #11083 worked, but was incomplete in that there was no mechanism by which the extension could notify an LS that the interpreter had changed and that its view of pythonPath needs to be updated. When pythonPath changes in the actual settings.json file, VS Code knows how to send configuration changes to the right parties, and we get that for free.

For now, manually trigger a didChangeConfiguration notification to ask the LS to check the settings again.

Unfortunately, this notification provides no way to indicate which scope has changed, so must indiscriminately notify all. This may be optimized in the future with a custom protocol to manage pythonPath changes with LSs.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@jakebailey jakebailey added the no-changelog No news entry required label Jun 15, 2020
@karrtikr
Copy link

Unfortunately, this notification provides no way to indicate which scope has changed, so must indiscriminately notify all. This may be optimized in the future with a custom protocol to manage pythonPath changes with LSs.

Curious, why do you need the scope information? I think what you need is the value of the selected interpreter, that's it.

@jakebailey
Copy link
Member Author

Curious, why do you need the scope information? I think what you need is the value of the selected interpreter, that's it.

The LSP's workspace/configuration call allows you to say "give me the configuration for this URI", handling multi-root workspaces. But the notification back to the LS is not constrained to any scope, meaning the server needs to requery everything to know what has changed per-workspace.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jakebailey jakebailey merged commit 1cd6bb7 into microsoft:master Jun 16, 2020
@jakebailey jakebailey deleted the interp-did-change branch June 16, 2020 18:58
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants