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

Clarify "defaultValue" in Workspace Configuration section #105598

Closed
RMacfarlane opened this issue Aug 28, 2020 · 6 comments
Closed

Clarify "defaultValue" in Workspace Configuration section #105598

RMacfarlane opened this issue Aug 28, 2020 · 6 comments
Assignees
Labels
config VS Code configuration, set up issues insiders-released Patch has been released in VS Code Insiders polish Cleanup and polish issue

Comments

@RMacfarlane
Copy link
Contributor

#105550

The documentation says explicitly that the effective value returned by WorkspaceConfiguration.get(section: string, defaultValue: T) is a merge of the inheritance for object value types, overrides for all other types.

Steps to Reproduce:

Provide {a: 1} as the second parameter of WorkspaceConfiguration.get(sectionL string, defaultValue: T)
Have {b: 2} in your user settings or workspace settings.
See the return is {b: 2}, not {a: 1, b: 2} as expected from reading the documentation.
Note that the docs may only be talking about defaults provided in package.json but that is not clear. Is the intent to have the defaultValue provided in the API overriden?

In https://code.visualstudio.com/api/references/vscode-api#WorkspaceConfiguration, we repeatedly use defaultValue in examples, referring to the default value the setting was created with. This behaves differently than the defaultValue parameter used when calling WorkspaceConfiguration.get. If I understand correctly, the former gets merged with other values in the settings hierarchy, and the latter is only used if the setting is undefined.

@gregvanl
Copy link

The documentation for vscode-api is auto-generated from https://github.com/microsoft/vscode/blob/master/src/vs/vscode.d.ts, so the updates would need to be made in the vscode repo.

@RMacfarlane
Copy link
Contributor Author

Oops, I will transfer it back then 😄

@RMacfarlane RMacfarlane transferred this issue from microsoft/vscode-docs Aug 28, 2020
@sandy081
Copy link
Member

sandy081 commented Sep 4, 2020

Not sure which default value you mean, if it is about following:

/**
 * Return a value from this configuration.
 *
 * @param section Configuration name, supports _dotted_ names.
 * @param defaultValue A value should be returned when no value could be found, is `undefined`.
 * @return The value `section` denotes or the default.
 */
get<T>(section: string, defaultValue: T): T;

As the documentation says above API returns the value for the given section, if there is no value found (ie., undefined) then passed in defaultValue is returned.

Let me know if there can be improvements, happy to do it.

@sandy081 sandy081 added the info-needed Issue requires more information from poster label Sep 4, 2020
@scottmwyant
Copy link

@sandy081 The defaultValue parameter you're showing is different than the defaultValue at the root of the config hierarchy shown here (screen shot below). The issue is that the same identifier is used for two different things and the documentation doesn't make a distinction. I thought that by providing an argument to this parameter I was setting the base value used to compute the "effective value". That is not the case.

The defaultValue parameter is not part of the "effective value" that is computed by WorkspaceConfiguration.get(). The only defaultValue that is considered when computing the "effective value" is defined in the extension's package.json. After thinking through this, it makes sense; you wouldn't want one extension to be able to set defaults for another extension...


image

@sandy081
Copy link
Member

sandy081 commented Sep 4, 2020

I see what you mean, the defaultValue in get<T>(section: string, defaultValue: T): T; is nothing to do with the defaultValue of the configuration. Not sure if I can change the names as they makes sense in the context in which they are mentioned. To be clear, I can add a comment next to the defaultValue in the screen shot above

@scottmwyant
Copy link

I think that would be appropriate. I might suggest "(from package.json)" over "(if defined)", which I'd be afraid may still lead someone to believe the defaultValue can be defined with the get method.

@sandy081 sandy081 added config VS Code configuration, set up issues polish Cleanup and polish issue and removed info-needed Issue requires more information from poster labels Sep 15, 2020
@sandy081 sandy081 added this to the September 2020 milestone Sep 15, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
config VS Code configuration, set up issues insiders-released Patch has been released in VS Code Insiders polish Cleanup and polish issue
Projects
None yet
Development

No branches or pull requests

5 participants
@RMacfarlane @sandy081 @gregvanl @scottmwyant and others