WorkspaceFolder.raw is weird

[bpasero]

See:

vscode/src/vs/platform/workspace/common/workspace.ts

Lines 334 to 337 in 2f17858

	constructor(
	data: IWorkspaceFolderData,
	readonly raw?: { path: string; name?: string } | { uri: string; name?: string }
	) {

There seems to be 2 competing parameters for what the folder is.

I found this while trying to untangle dependencies from workspace to workspaces and had to inline this one.

[sandy081]

Its stored data and used proper typings

[bpasero]

@sandy081 but this is now bringing back a cyclic dependency between workspace and workspaces. I tried to avoid that, so this interface cannot be in workspaces.

[sandy081]

Ah ok, I did not know about that.

[sandy081]

It seems workspace related types shall be defined in workspace file. So moved raw types to workspace file to remove cyclic dependencies. Feel free to move all other workspace related types to workspace file.

[bpasero]

@sandy081 I still do not understand why a WorkspaceFolder has:

• IWorkspaceFolderData which has a uri, name and index
• raw which has uri, name and path

This sounds like 2 kinds of sources for what a workspace folder is? Why is that?

If you somehow need this original raw information in your configuration service then I would rather suggest to add a new method to the workspace folder that answers exactly the question you ask and not make it a public property.

[sandy081]

IWorkspaceFolderData is the normalized data computed from IRawFileWorkspaceFolder | IRawUriWorkspaceFolder. Please see this:

vscode/src/vs/platform/workspaces/common/workspaces.ts

Line 197 in 1d79533

export function toWorkspaceFolders(configuredFolders: IStoredWorkspaceFolder[], workspaceConfigFile: URI, extUri: IExtUri): WorkspaceFolder[] {

I need raw data (truth) while updating the workspace configuration file, for eg when updating/adding/removing a workspace folder so that the update is not changing any underlying semantics of that specific workspace folder.

Otherwise, I would have to create another class extending WorkspaceFolder to store this info. I can do it but may not worth.

[bpasero]

The only client seems to be this code:

vscode/src/vs/workbench/services/configuration/browser/configurationService.ts

Lines 223 to 229 in 1d79533

	let newStoredFolders: IStoredWorkspaceFolder[] = currentWorkspaceFolders.map(f => f.raw).filter((folder, index): folder is IStoredWorkspaceFolder => {
	if (!isStoredWorkspaceFolder(folder)) {
	return true; // keep entries which are unrelated
	}
	return !this.contains(foldersToRemove, currentWorkspaceFolders[index].uri); // keep entries which are unrelated
	});

Can it a) move into the WorkspaceFolder somehow as a method to keep access to the raw data private or be done with the other data from the workspace?

[sandy081]

Not both options are possible. As said the better option would be to have a _WorkspaceFolder class that extends WorkspaceFolder in ConfigurationService and wrap this information. May I know what's the harm in leaving it there?

[bpasero]

The part that imho is debt is around the fact that creating WorkspaceFolder asks to pass in:

• data: IWorkspaceFolderData
• raw?: IRawFileWorkspaceFolder | IRawUriWorkspaceFolder

I would have expected here that we only asks to pass in the IWorkspaceFolderData to know what the folder is about. Because if I pass in both things, how does the WorkspaceFolder pick its meta data from? Does it use data or does it use raw?

And similar, when I am using the WorkspaceFolder, should I get the meta data from raw or not?

If we somehow need to signal that the WorkspaceFolder was constructed from a stored representation then maybe that should be a subclass which is then only created from the raw data and not asking to pass in IWorkspaceFolderData

[sandy081]

WorkspaceFolder populate its metadata using IWorkspaceFolderData and exposes its raw/source information IStoredWorkspaceFolder for those who need it (like workspace editing). I do not see this as a major debt that has to be restructured .

FYI WorkspaceFolder cannot be constructed itself from IStoredWorkspaceFolder because it needs additional info like Workspace config path. Hence we are computing it outside. Please see

vscode/src/vs/platform/workspaces/common/workspaces.ts

Line 197 in 1d79533

export function toWorkspaceFolders(configuredFolders: IStoredWorkspaceFolder[], workspaceConfigFile: URI, extUri: IExtUri): WorkspaceFolder[] {

If you think you are not sure what's raw property is, then I would rename it to source and probably add a comment instead of doing a refactoring. I am sorry that I would not do more than that.

[bpasero]

@sandy081 why do we need access to the raw data to begin with? Is there any property in raw that cannot be figured out from the properties of WorkspaceFolder? Isn't this typically true:

workspaceFolder.name === workspaceFolder.raw.name
workspaceFolder.uri.toString() === workspaceFolder.raw.uri
workspaceFolder.uri.path === workspaceFolder.raw.path

?

[sandy081]

raw paths are not same as the computed uri path, because the raw path can be relative and we also normalize the URIs. Name is also optional and we compute it. Please refer to this code

vscode/src/vs/platform/workspaces/common/workspaces.ts

Lines 197 to 233 in 99cfdd5

	export function toWorkspaceFolders(configuredFolders: IStoredWorkspaceFolder[], workspaceConfigFile: URI, extUri: IExtUri): WorkspaceFolder[] {
	let result: WorkspaceFolder[] = [];
	let seen: Set<string> = new Set();
	const relativeTo = extUri.dirname(workspaceConfigFile);
	for (let configuredFolder of configuredFolders) {
	let uri: URI | undefined = undefined;
	if (isRawFileWorkspaceFolder(configuredFolder)) {
	if (configuredFolder.path) {
	uri = extUri.resolvePath(relativeTo, configuredFolder.path);
	}
	} else if (isRawUriWorkspaceFolder(configuredFolder)) {
	try {
	uri = URI.parse(configuredFolder.uri);
	if (uri.path[0] !== '/') {
	uri = uri.with({ path: '/' + uri.path }); // this makes sure all workspace folder are absolute
	}
	} catch (e) {
	console.warn(e); // ignore
	}
	}
	if (uri) {
	// remove duplicates
	let comparisonKey = extUri.getComparisonKey(uri);
	if (!seen.has(comparisonKey)) {
	seen.add(comparisonKey);
	const name = configuredFolder.name || extUri.basenameOrAuthority(uri);
	result.push(new WorkspaceFolder({ uri, name, index: result.length }, configuredFolder));
	}
	}
	}
	return result;
	}

[bpasero]

That is something we should document, pushed it:

vscode/src/vs/platform/workspace/common/workspace.ts

Lines 385 to 392 in 7141d2a

	/**
	* Provides access to the original metadata for this workspace
	* folder. This can be different from the metadata provided in
	* this class:
	* - raw paths can be relative
	* - raw paths are not normalized
	*/
	readonly raw?: IRawFileWorkspaceFolder | IRawUriWorkspaceFolder