Skip to content

Enable pythonPath deprecation experiment to 4% of users #12002

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

Merged
merged 1 commit into from
May 27, 2020

Conversation

luabud
Copy link
Member

@luabud luabud commented May 26, 2020

To enable experiment for #2125

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.

@luabud luabud added the no-changelog No news entry required label May 26, 2020
@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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@luabud luabud marked this pull request as ready for review May 27, 2020 00:35
@luabud luabud requested a review from karthiknadig May 27, 2020 00:36
@karthiknadig
Copy link
Member

Since this experiment has not yet started. Should we be using the new framework?

@luabud
Copy link
Member Author

luabud commented May 27, 2020

It'll take a couple of weeks until we'd be able to turn it on with the new framework. I'd rather start with this one and leave it as the last experiment with our old framework.

@luabud luabud merged commit 8b83f40 into microsoft:master May 27, 2020
@luabud luabud deleted the enablepythonpath branch May 27, 2020 23:28
},
{
"name": "DeprecatePythonPath - control",
"salt": "DeprecatePythonPath",
"min": 100,
"max": 100
"max": 96

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min should be 96 and max should be 100 instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karrtikr Good catch

@luabud luabud restored the enablepythonpath branch May 28, 2020 19:12
@zor-el
Copy link

zor-el commented Jun 12, 2020

I'm posting this coming from #2125 because that issue is referenced from the announcing blog post.

Complete removal of python.pythonPath from the settings is a massively breaking change that IMO isn't warranted by the #2125 issue that has been used to justify it in the above blog post. (This change has certainly been breaking things left and right for me.) The explanation in that blog post's comment that "it can be useful" doesn't invalidate my argument. A breaking change is harmful, so it should be significantly outweighed by a benefit that cannot by obtained otherwise. This is not the case here though, IMO.

Although it is true that pythonPath would likely be different for different people, I'd argue that is true for the entire (or most of) .vscode/settings.json. For example, extensions can and do store settings in .vscode/settings.json that may just as well conflict with other users.

Settings are not generally transferable from user to user and from machine to machine, so there is little point in sharing them. In those cases that one would want to share them, one would certainly want to separate transferable from non-transferable settings.

Note the OP's issue wasn't the existence of python.pythonPath, but the unsolicited modification of .vscode/settings.json, which sneaks in non-transferable content into (in their case) shared settings.

Apparently the underlying issue is the lack of means for separation of transferable from non-transferable settings. That separation is inadequately addressed by removal of pythonPath (it is a general issue, not one specific to Python). The concrete issue #2125 is not about pythonPath removal either but about unsolicited modification of .vscode/settings.json. That is just as inadequately addressed by mere removal of pythonPath (IMO it could have been solved without breaking everybody's settings).

Question: Why can we not keep python.pythonPath for backward compatibility and simply ask the user for permission to overwrite it (and hint at the fact that the setting may not be transferable)?

Perhaps even the experiment's pythonPath caching behavior could remain intact, but an explicit pythonPath setting could take precedence?

@karrtikr
Copy link

karrtikr commented Jun 12, 2020

Opened up a new issue #12313 to answer your queries, and I recommend opening new issues for how removing this setting is breaking things for you.

@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