fix: renamed context to extensionContext #4165
Conversation
| @@ -11,8 +11,7 @@ import { expect } from 'chai'; | |||
| import * as path from 'path'; | |||
| import { createSandbox, SinonStub } from 'sinon'; | |||
| import { LibrarySourceRetrieveManifestExecutor } from '../../../src/commands/forceSourceRetrieveManifest'; | |||
| import { workspaceContext } from '../../../src/context'; | |||
| import { nls } from '../../../src/messages'; | |||
There was a problem hiding this comment.
@jeffb-sfdc was NLS meant to be removed here too?
There was a problem hiding this comment.
nls isn't used, so I removed it since workspaceContext (also not used) was removed.
| @@ -80,7 +80,7 @@ class MockEnvironmentVariableCollection | |||
| } | |||
| } | |||
|
|
|||
| export class MockContext implements ExtensionContext { | |||
| export class MockExtensionContext implements ExtensionContext { | |||
There was a problem hiding this comment.
I actually do really like this change here - much clearer what/why we're mocking 👍
| @@ -4,24 +4,26 @@ import * as vscode from 'vscode'; | |||
| import sinon, { stubInterface, stubObject } from 'ts-sinon'; | |||
| import * as sinonChai from 'sinon-chai'; | |||
| import { shared as lspCommon } from '@salesforce/lightning-lsp-common'; | |||
| import { MockContext } from './MockContext'; | |||
| import { MockExtensionContext } from './MockExtensionContext'; | |||
There was a problem hiding this comment.
Just checking that the directory was renamed here as well? This was the case for all of the MockContext's?
There was a problem hiding this comment.
No, just the file was renamed. The path is packages/salesforcedx-vscode-lwc/test/vscode-integration/activation/MockExtensionContext.ts, and the parent directory, activation was not changed, just the filename.
There was a problem hiding this comment.
Those are all my questions - I think refactoring here makes it much easier to understand the different type of contexts used, especially as folks onboard to the product. Perhaps it was clearer to previous maintainers what the differences were, but I'm supportive of changes that make we think will make it easier to develop going forward! Given that builds are working and tests are passing, I'm ok approving once @klewis-sfdc has reviewed them with you as well. Maybe we can plan for a larger spot check during next week's release too?
There was a problem hiding this comment.
Thanks for the PR, Jeff! Overall, I really like the changes from
- 👍🏽 "context" --> "extensionContext" and
- 👍🏽 "MockContext" --> "MockExtensionContext",
but I'm not as in agreement with the changes to
⚠️ add "Instance" into var names.
"Instance" is a technical implementation detail that IMO does not belong in the var name. I actually prefer the expression on the left side here:
I would prefer not to establish or continue this naming convention. Let me know what you think and how strongly you feel about wanting that change. Happy to discuss more in standup today!
@klewis-sfdc done |
…nd-WorkspaceContext
jeffb-sfdc
deleted the
jb/refactor-context-to-ExtensionContext-and-WorkspaceContext
branch
What does this PR do?
This PR renames instances of "context" to "extensionContext" to avoid confusion between ExtensionContext and WorkspaceContext variables.
What issues does this PR fix or reference?
#@W-11171762@
Functionality Before / Functionality After
No functionality change