CHANGE: @W-16092798@: Introduce Workspace and update rule selection to take in a workspace #32
Conversation
stephen-carter-at-sf
force-pushed
the
sc/W-16092798
branch
3 times, most recently
from
8ce56f2 to
ec3c582
Compare
stephen-carter-at-sf
force-pushed
the
sc/W-16092798
branch
from
ec3c582 to
ead5fa6
Compare
| this.uniqueIdGenerator = uniqueIdGenerator; | ||
| } | ||
|
|
||
| public async createWorkspace(filesAndFolders: string[]): Promise<Workspace> { |
There was a problem hiding this comment.
Clients will need to use this to create a workspace first before they can call selectRules or runRules.
| async getExpandedFiles(): Promise<string[]> { | ||
| if (!this.expandedFiles) { | ||
| this.expandedFiles = await expandToListAllFiles(this.filesAndFolders); | ||
| } | ||
| return this.expandedFiles as string[]; | ||
| } |
There was a problem hiding this comment.
This is where we get a big win. Instead of having each engine to do this themselves, the workspace object that they receive will be able to do this for them and cache the information so that other engines won't have to do it yet again.
| @@ -16,3 +18,42 @@ export function changeWorkingDirectoryToPackageRoot() { | |||
| process.chdir(original_working_directory); | |||
| }); | |||
| } | |||
|
|
|||
| export class WorkspaceForTesting implements Workspace { | |||
There was a problem hiding this comment.
Eventually all these test tools for each of the various engines should probably end up in their own package. At that point maybe we'd call it @salesforce/code-analyzer-engine-test-helpers or something. Until then I'm fine living with the redundant code.
There was a problem hiding this comment.
Question: How feasible would it be to move these into the core module instead? Yeah, it's not their final destination, but it still cuts down the redundant code and minimizes the risk of divergence.
There was a problem hiding this comment.
The engines don't and shouldn't depend on core. So either we move these into the engine-api or we put it into a test package. But yeah maybe the engine-api can have test helpers. I try not to bloat an api module though... so we'll defer this decision for now.
stephen-carter-at-sf
changed the title
| @@ -13,4 +13,16 @@ export class RealClock implements Clock { | |||
| now(): Date { | |||
| return new Date(); | |||
| } | |||
| } | |||
|
|
|||
| export interface UniqueIdGenerator { | |||
There was a problem hiding this comment.
I've typically used uuid for this sort of thing.
There was a problem hiding this comment.
The issue with uuid or any other utility is that for testing, I might need to fix the generated ids when doing equality checking. Same issue with clock or random number generator. Have to be able to fix things. Because I needed an interface anyway, a so I went with a simple counter approach.
|
|
||
| expect(() => selection.getRule('stubEngine1', 'doesNotExist')).toThrow( | ||
| getMessage('RuleDoesNotExistInSelection', 'doesNotExist', 'stubEngine1')); | ||
| expect(() => selection.getRule('oopsEngine', 'stub1RuleD')).toThrow( | ||
| getMessage('RuleDoesNotExistInSelection', 'stub1RuleD', 'oopsEngine')); | ||
| }); | ||
|
|
||
| it('When an engine returns multiple rules with the same name, then error', async () => { |
There was a problem hiding this comment.
If I'm understanding correctly, this test got moved from add-engines to here because adding engines no longer intrinsically fetches all the rules from the engine the way it used to?
There was a problem hiding this comment.
That's right. I didn't remove any tests actually - they all got moved. And your understanding is spot on.
| @@ -16,3 +18,42 @@ export function changeWorkingDirectoryToPackageRoot() { | |||
| process.chdir(original_working_directory); | |||
| }); | |||
| } | |||
|
|
|||
| export class WorkspaceForTesting implements Workspace { | |||
There was a problem hiding this comment.
Question: How feasible would it be to move these into the core module instead? Yeah, it's not their final destination, but it still cuts down the redundant code and minimizes the risk of divergence.
No description provided.