Skip to content

Conversation

@liorfr
Copy link
Contributor

@liorfr liorfr commented Feb 26, 2021

No description provided.

@liorfr liorfr requested a review from allacherkes February 26, 2021 15:18
abstract isEnabled(parameters: AppStudioMultiParameters, context: AppStudioMultiContext): boolean;
}

export function registerStrategies(appStudioMultiStrategy: AppStudioMultiStrategy): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method doesnt feel in the right place.
You define abstract class here for strategy and then comes this specific method which use instances of classes which implement this abstract class.
I would do it as instance method of AppStudioMultiStrategy class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppStudioMultiStrategy is a generic class that should not know the concrete implementation.
unleash_client_manager.ts manage AppStudioMultiStrategy, initialize and register the strategies

const client = await initializeUnleashClient(extensionName, serverArgs, [new AppStudioMultiStrategy()]);
const appStudioMultiStrategy = new AppStudioMultiStrategy();
const client = await initializeUnleashClient(extensionName, serverArgs, [appStudioMultiStrategy]);
registerStrategies(appStudioMultiStrategy);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer instance method: appStudioMultiStrategy.registerStrategies()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppStudioMultiStrategy is a generic class that should not know the concrete implementation.
unleash_client_manager.ts manage AppStudioMultiStrategy, initialize and register the strategies

@@ -1,11 +1,14 @@
import { expect } from "chai";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename the file to strategy.spec.ts as it is testing several files inside strategy folder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@liorfr liorfr merged commit 7745cf8 into master Mar 15, 2021
@liorfr liorfr deleted the stratRefactoring branch March 15, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants