🔧🚗 Migrate setting value of codeFormatting.whitespaceAroundPipe (if present) to new setting codeFormatting.addWhitespaceAroundPipe automatically. #2689
Conversation
…etting codeFormatting.addWhitespaceAroundPipe automatically.
| return configurationTarget; | ||
| } | ||
|
|
||
| export async function change( |
There was a problem hiding this comment.
this function is functionally still the same , only the configurationTarget parameter has been optimized to match exactly the called VS-Code API underneath.
| @@ -320,6 +322,18 @@ export class SessionManager implements Middleware { | |||
| return resolvedCodeLens; | |||
| } | |||
|
|
|||
| // During preview, populate a new setting value but not remove the old value. | |||
There was a problem hiding this comment.
I think we may want to do this for the next stable release as well since a lot of customers only use stable.
After the next stable version, we can remove the old setting.
There was a problem hiding this comment.
Yes, that's exactly what I try to say: add new setting during preview but leave old setting still there (people who switch between preview/stable or use settings synchronization). Just before the next stable or in the last preview, I'd modify to to additionally remove the old setting. Is that clearer? But it sounds like we are both on the same page.
There was a problem hiding this comment.
Yes I think so. Can you capture this in an issue so we don't forget?
I'd even suggest adding a TODO with the issue number... That way if we all disappear, someone could pickup where we left off
There was a problem hiding this comment.
I opened issue #2693 and will add a comment
| @@ -320,6 +322,18 @@ export class SessionManager implements Middleware { | |||
| return resolvedCodeLens; | |||
| } | |||
|
|
|||
| // During preview, populate a new setting value but not remove the old value. | |||
| // When the PowerShell extension releases the RTM version, then the old value can be safely removed. | |||
| private async migrateWhitespaceAroundPipeSetting() { | |||
There was a problem hiding this comment.
Do you wanna make this more of a utility function that takes in the old an new setting?
That'll be handy in the future.
There was a problem hiding this comment.
I'd rather defer it to when it is needed as it might be a YAGNI.
Or maybe I should create an NPM package for it so that I can break the Internet later one 😂
https://www.zdnet.com/article/another-one-line-npm-package-breaks-the-javascript-ecosystem/
src/settings.ts
Outdated
| const configuration: vscode.WorkspaceConfiguration = | ||
| vscode.workspace.getConfiguration( | ||
| utils.PowerShellLanguageId); | ||
| export async function getConfigurationTarget(settingName: string): Promise<vscode.ConfigurationTarget> { |
There was a problem hiding this comment.
This is to get where the setting is currently set, yes?
A small comment would be appreciated
There was a problem hiding this comment.
Yes, it gets the scope (they call it ConfigurationTarget) from where the effective setting is coming from (workspace settings override user settings, hence why the order is important here). If someone defined the old setting on both levels, we only look at the one that determines the effective setting and change that one. I'll add a comment and rename the function to getEffectiveConfigurationTarget to make it clearer.
| @@ -251,21 +251,22 @@ export async function getEffectiveConfigurationTarget(settingName: string): Prom | |||
|
|
|||
| const detail = configuration.inspect(settingName); | |||
| let configurationTarget = null; | |||
| if (typeof detail.workspaceFolderValue !== undefined) { | |||
| if (typeof detail.workspaceFolderValue !== "undefined") { | |||
There was a problem hiding this comment.
should this be this?
| if (typeof detail.workspaceFolderValue !== "undefined") { | |
| if (detail.workspaceFolderValue !== undefined) { |
There was a problem hiding this comment.
No, Codacy/ESLint will complain because of not using typeof and not using double quotes around undefined:
https://eslint.org/docs/rules/no-undefined
Unfortunately, Codacy seems to lose its history but those were the complaints that made the build red in my previous commits.
There was a problem hiding this comment.
I read the link. I think that advice is fine.
PR Summary
This for the time of the preview with PSSA 1.19.0 to help migrate the setting automatically.
Because of settings sync, I do not want to remove the old setting yet, we should add removal of the old setting only for the RTM version of the PowerShell extension (where we will then also be able to give an informational window to the user)
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready