Clean up setup internals #3435
Clean up setup internals #3435
Conversation
mattseddon
force-pushed
the
refactor-setup
branch
from
53e1513 to
51e849b
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
mattseddon
force-pushed
the
refactor-setup
branch
2 times, most recently
from
7e38c3e to
6440b70
Compare
| @@ -131,6 +133,8 @@ export class Extension extends Disposable { | |||
| new InternalCommands(outputChannel, ...clis) | |||
| ) | |||
|
|
|||
| const status = this.dispose.track(new Status(config, ...clis)) | |||
There was a problem hiding this comment.
[F] There was a bug in the way that we were creating Status it did not contain the dvcViewer.
There was a problem hiding this comment.
I'll add a type in a quick follow-up
| export const buildSetup = ( | ||
| disposer: Disposer, | ||
| dependencies?: { |
There was a problem hiding this comment.
[F] This was not used anywhere
| dvcReader: DvcReader, | ||
| dvcRunner: DvcRunner, | ||
| gitExecutor: GitExecutor, | ||
| gitReader: GitReader, |
There was a problem hiding this comment.
[F] All of these IClis sit behind the InternalCommands facade. No point to include the functionality twice.
| } | ||
| } | ||
|
|
||
| public async setupWorkspace() { |
There was a problem hiding this comment.
[F] move from private to public only, also done so it can be registered
| @@ -257,10 +241,50 @@ export class Setup | |||
| }) | |||
| } | |||
|
|
|||
| public async selectFocusedProjects() { | |||
There was a problem hiding this comment.
[F] exposed so it can be registered as a command
| @@ -310,13 +337,6 @@ export class Setup | |||
| } | |||
| } | |||
|
|
|||
| private async initializeDvc() { | |||
There was a problem hiding this comment.
[F] Registered externally and used in that way so we get a "free" telemetry event
| @@ -36,7 +36,7 @@ export const initializeEmptyRepo = async (): Promise<string> => { | |||
| return '' | |||
| } | |||
|
|
|||
| await gitExecutor.init(TEMP_DIR) | |||
| await gitExecutor.gitInit(TEMP_DIR) | |||
There was a problem hiding this comment.
[F] (unfortunate but) you can't register the same command name twice in InternalCommands
| mockDvcRoot: string | undefined, | ||
| mockGitRoot: string | undefined, | ||
| noGitCommits = true | ||
| ) => { |
There was a problem hiding this comment.
[F] Now that we are using InternalCommands we have to build the dependencies in the normal way.
mattseddon
force-pushed
the
refactor-setup
branch
from
b698552 to
c5f7eaa
Compare
| return await this.internalCommands.executeCommand( | ||
| AvailableCommands.VERSION, | ||
| cwd | ||
| ) |
There was a problem hiding this comment.
[F] Move the logic out of the "dumb" reader and into here. Wrote some tests to cover it.
|
Code Climate has analyzed commit 5198cae and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 89.4% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.7% (0.0% change). View more on Code Climate. |
Related to #3434
This PR starts to clean-up the internals of
Setup. Comments are inline but the main change is removing instances ofDvcReader,DvcExecutor,GitReader&GitExecutorfromSetup. All of the required commands should be available throughInternalCommandswhich makes things in the class a bit cleaner.I also moved command registration out of the class (usual pattern) as then we don't have to worry about commands being "re-registered"/throwing errors when we build
Setupfor testing purposes.