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

Revisit getWorkspace-API #34843

Closed
jrieken opened this issue Sep 22, 2017 · 8 comments
Closed

Revisit getWorkspace-API #34843

jrieken opened this issue Sep 22, 2017 · 8 comments
Assignees
Labels
api under-discussion Issue is under discussion for relevance, priority, approach workbench-multiroot Multi-root (multiple folders) issues

Comments

@jrieken
Copy link
Member

jrieken commented Sep 22, 2017

We have added the getWorkspace-api with some special behaviour in case it's called with the path of a workspace. Not every expected that it behaves like and we should (1) have a flag to make it behave different, e.g. return the workspace folder if called with one and (2) consider changing/breaking the defaults

@jrieken jrieken self-assigned this Sep 22, 2017
@vscodebot vscodebot bot added the api label Sep 22, 2017
@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach workbench-multiroot Multi-root (multiple folders) issues labels Sep 22, 2017
@jrieken jrieken added this to the September 2017 milestone Sep 22, 2017
@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2017

@sandy081 Is your tool ready to check on usages of this?

@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2017

cc @bpasero, @dbaeumer

@bpasero
Copy link
Member

bpasero commented Sep 27, 2017

+1 for changing the default behaviour and adding that option for the current behaviour.

@sandy081
Copy link
Member

@jrieken Not yet. Will be ready by tomorrow.

@dbaeumer
Copy link
Member

+1 for changing this.

@aeschli and I once had a discussion how to provide this in the LSP (which we didn't at the end) and one idea was that getWorkspaceFolder returns the folder if called with one and that we have a second method getParentWorkspaceFolders which returns an array of all parent folders. The idea was that if I am interested in nesting I am very likely interested in all parents or the top most parent. That would avoid having a flag and looping.

@jrieken
Copy link
Member Author

jrieken commented Sep 28, 2017

I went through the list of extensions using this and how they use it. I was not able to find the sources of two but all the others use getWorkspaceFolder with a document-uri. That means they won't be affected by the planned change.

This is what I am planning to do (when being called with a workspace folder)

  1. Return workspace folder if uri points to a workspace folder and if resolveParent is falsy
  2. Return undefined or parent workspace folder if uri points to a workspace folder and if resolveParent is true (to today behaviour)

Update: made this more simple, in the spirit of #34843 (comment) and removed the "return parent workspace folder" semantics

export function getWorkspaceFolder(uri: Uri): WorkspaceFolder | undefined;

@bpasero
Copy link
Member

bpasero commented Sep 28, 2017

Makes sense, though the name resolveParent implies to me that there is some actual work behind it (even though its not), maybe a better name could be found.

@jrieken
Copy link
Member Author

jrieken commented Sep 28, 2017

Yeah, nuked the parent-stuff all together

@microsoft microsoft deleted a comment from sandy081 Sep 28, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api under-discussion Issue is under discussion for relevance, priority, approach workbench-multiroot Multi-root (multiple folders) issues
Projects
None yet
Development

No branches or pull requests

4 participants