-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Ensure workspace is read from WorkspaceContextService #17494
Conversation
Hi @rothfels, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience! |
Hi @rothfels, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
3410246
to
25dcdbb
Compare
Not convinced that justified such a change. |
Me neither, I think it is fine to pass around workspace object instead of using the context service. Though in the future we might have to revisit this when we support multiple workspaces in one window. |
Fair, though the secondary motivation is what @bpasero described. I figured this might not be on the roadmap so a consistency argument would make more sense. But for context, I'm running a patched version of vscode which loads a workbench in the user's web browser, similar to https://github.com/spiffcode/ghedit. We provide a "go-to-definition" feature which can navigate the user to a different workspace (e.g. for external dependencies). For the in-browser use case, we are forced to work with one window. Centralizing access to workspace state allows us in our patch to mutate that state exactly once. I can see this change being unnecessary if you don't intend to support multiple workspaces in one window. |
LGTM, thanks. |
In most of the codebase (examples linked below), the services, parts, etc which depend on workspace context declare that through the dependency injection system, and always read workspace from the WorkspaceContextService (versus some cached instance state). This change replaces hard wires with getter method calls on the injected WorkspaceContextService, and prefers always using these method calls to read workspace. The primary motivation for the change is consistency.
Examples from past commits of the appropriate pattern: