Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

ignoreSync for path settings element #6474

Closed
Malix-Labs opened this issue Sep 28, 2024 · 6 comments
Closed

ignoreSync for path settings element #6474

Malix-Labs opened this issue Sep 28, 2024 · 6 comments
Assignees
Labels
needs decision Do we want this enhancement? needs repro Issue has not been reproduced yet

Comments

@Malix-Labs
Copy link

Problem

Some settings are for specifying local paths, which could be different for each user's machine

Solution

Add "ignoreSync" : true to them

Related

@erictraut
Copy link
Contributor

I'd want to wait until this is officially supported and documented in the VS Code specs. It looks like it's an undocumented (and perhaps unspecified?) feature in VS Code currently.

I'd also want to make sure that this is consistent between the pyright LSP and Pylance, so the Pylance team members should weigh in on this as well. @debonte, @rchiodo, do you have any thoughts on this proposed change?

@Malix-Labs
Copy link
Author

Malix-Labs commented Sep 28, 2024

It's used in some built-in vscode extensions since quite some time

The documentation might have been forgotten, I guess, hence why I oppened microsoft/vscode-docs#7638.

This is what it looks like from the LSP :

LSP

Also note that I didn't check if there were some missing ignoreSync in Pylance too (close-source)

@debonte
Copy link
Contributor

debonte commented Sep 28, 2024

While I see the rationale here, I wonder:

  1. How many people are setting these settings at the user level? I would expect them to be more typically applied at the workspace level. Perhaps we can tell from telemetry.
  2. How many people who have set these settings at the user level are relying on the current behavior because they set up their machines in a consistent manner and therefore syncing the paths makes sense?
  3. If the number of people negatively impacted by the current behavior is small, they could opt these settings out of settings sync via settingsSync.ignoredSettings. That said, we could also disable syncing of these settings by default and then require users who want syncing to opt-in, but I find that ability less discoverable.

@Malix-Labs, thanks for opening the vscode-docs issue. I also couldn't find ignoreSync in the docs, although it was mentioned in the changelog in Jan 2023 for what that's worth: https://code.visualstudio.com/updates/v1_75#_ignore-a-setting-to-sync

Using the "machine" scope for a config setting apparently also prevents syncing by default. I did notice that Pylance's python.analysis.nodeExecutable setting is machine-scoped, but that was for security reasons, so I don't see it as a precedent.

@Malix-Labs
Copy link
Author

How many people are setting these settings at the user level? I would expect them to be more typically applied at the workspace level. Perhaps we can tell from telemetry.

"ignoreSync" only applies for user-level, correct;
So the edits must be seen as edits given a user-level scope

How many people who have set these settings at the user level are relying on the current behavior because they set up their machines in a consistent manner and therefore syncing the paths makes sense?

It won't break current configurations, just won't sync after that
Though, it can be reactivated by manually enabling back the sync

we could also disable syncing of these settings by default and then require users who want syncing to opt-in, but I find that ability less discoverable.

This is what ignoreSync is, indeed

although it was mentioned in the changelog in Jan 2023 for what that's worth: code.visualstudio.com/updates/v1_75#_ignore-a-setting-to-sync

Yep, I mentioned that in the issue already

Using the "machine" scope for a config setting apparently also prevents syncing by default.

I didn't know that however, thanks for the notice !

@erictraut
Copy link
Contributor

Thanks for the additional information and analysis.

I don't have a strong opinion about whether this change should be made in pyright. My only stipulation is that I think it should match pylance in this regard. I'll leave it to the pylance team to decide whether they want to make this change in pylance — and if so, to make the similar change in pyright.

@debonte, if you're good with that resolution, then I suggest that we move this enhancement request to the pylance-release project so it can be triaged, prioritized, and tracked by the pylance team.

@erictraut erictraut added the needs decision Do we want this enhancement? label Sep 28, 2024
@debonte debonte transferred this issue from microsoft/pyright Sep 28, 2024
@github-actions github-actions bot added the needs repro Issue has not been reproduced yet label Sep 28, 2024
@debonte
Copy link
Contributor

debonte commented Sep 28, 2024

Moving this issue to discussions as an enhancement request for comments and upvotes.

@microsoft microsoft locked and limited conversation to collaborators Sep 28, 2024
@debonte debonte converted this issue into discussion #6475 Sep 28, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
needs decision Do we want this enhancement? needs repro Issue has not been reproduced yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants