Introduce extend api catalog method #34

LironMShemen · 2025-01-06T14:24:21Z

No description provided.

Add the function to types.ts and index.ts

- Add `extendAPICategories` function to `PromptCreator` - Add `extendJSContext` function to `StepPerformer - Add `extendAPICatalog` function to `Copilot`

suitable test suites. test: suitable test suites. test: add suitable test suites - Add `Copilot`, `StepPerformer`, `index` and `PromptCreator` suites - Add utils file - Add snapshot file for `PromptCreator` test

asafkorem

Good job 🙌🏼 👸🏼
Push the fixes in a separate commit for now, we'll rebase things once we'll finish the review

asafkorem · 2025-01-06T15:57:15Z

src/Copilot.ts

+    private config: Config;

    private constructor(config: Config) {
+        this.config = config;


why do you need to save the config?

I don’t need it :)
Maybe I thought of it at the beginning.
Removed.

asafkorem · 2025-01-06T18:00:30Z

src/Copilot.test.ts

+    actionsCategory2,
+    actionsCategory,
+    tapButtonContext
+} from "./test-utils/expendAPICatalogUtils";


typo (extend*)

asafkorem · 2025-01-06T18:01:47Z

src/Copilot.test.ts

+            instance.extendAPICatalog(actionsCategory)
+            instance.extendAPICatalog(actionsCategory2, tapButtonContext);
+            expect(mockConfig.frameworkDriver.apiCatalog.categories).toEqual([...actionsCategory]);
+            expect(mockConfig.frameworkDriver.apiCatalog.categories[0].items).toHaveLength(2);


I think you dont need this check since you already checked equality

asafkorem · 2025-01-06T18:02:30Z

src/test-utils/expendAPICatalogUtils.ts

+    ]
+}];
+
+export const actionsCategory = [{


why as a list and not just an object?

You're right. Initially, I wanted to allow the user to add more than one category, so I let them insert a list of categories. I have now added the option to insert either a single Category object or a list of Categories.

asafkorem · 2025-01-06T18:06:17Z

src/test-utils/expendAPICatalogUtils.ts

+export const tapButtonContext = {tapButton: 'new tap button'};
+export const functionContext = {function: 'newFlow'};
+export const newFunctionContext = {function: 'newFunction'};


why do you need three different?
you can just use jest.fn()s instead

asafkorem · 2025-01-06T18:08:35Z

src/Copilot.test.ts

+            // item is added to existing category
+            instance.extendAPICatalog(actionsCategory)
+            instance.extendAPICatalog(actionsCategory2, tapButtonContext);
+            expect(mockConfig.frameworkDriver.apiCatalog.categories).toEqual([...actionsCategory]);


it looks like a bug 🤔
we need to merge categories in case they have the same name like in this example (both has the name Actions).

I think this is exactly what happens :)
You can see in the utils file that actionCategory and actionCategory2 have the same title, Actions.

asafkorem · 2025-01-06T18:09:05Z

src/Copilot.test.ts

+            expect(spyStepPerformer).toHaveBeenCalledWith(tapButtonContext);
+        });
+
+        it('should expend the API catalog with an existing category and an additional new category', () => {


typo (extend)

fix: changes to allow the function to get either a list of categories or a single category object

…t test

asafkorem

good job. round 2

asafkorem · 2025-01-07T12:35:10Z

src/Copilot.test.ts

+    customActionsCategory,
+    actionsCategory2,
+    actionsCategory,


naming is confusing (both refer to actions)
use barCategory and bazCategory1, bazCategory2 or something? just not to mention actions in both categories

asafkorem · 2025-01-07T12:36:11Z

src/Copilot.ts

@@ -109,6 +109,17 @@ export class Copilot {
            this.cacheHandler.flushTemporaryCache();
    }

+    /**
+     * Allow the user to add context and categories (titles and items) to the existing API catalog.


Enriches the API catalog with given categories and JS context.

or "Adds ...".
the description should describe what the method does (its effect)

asafkorem · 2025-01-07T12:38:28Z

src/actions/StepPerformer.test.ts

+            expect(mockCodeEvaluator.evaluate).toHaveBeenCalledWith(PROMPT_RESULT, dummyBarContext1);
+
+            // Extended context
+            const newFixedContext = { ...dummyBarContext1, ...dummyContext };


I think expectedContext or extendedContext is more semantically correct here

asafkorem · 2025-01-07T12:41:23Z

src/actions/StepPerformer.ts

+    extendJSContext(newContext: any): void {
+        for (const key in newContext) {
+            if (key in this.context) {
+                console.log(`Notice: Context ${key} is overridden by the new context value`);
+                break;
+            }
+        }
+        this.context = {...this.context, ...newContext};
+    }


extendJSContext(newContext: any): void { const overriddenKey = Object.keys(newContext).find(key => key in this.context); if (overriddenKey) { console.log(`Notice: Context ${overriddenKey} is overridden by the new context value`); } this.context = { ...this.context, ...newContext, }; }

Thinking about this now, we're not merging categories on the first initialization if user passes two categories with the same name.
It makes sense to me if we'll do the same thing there. But IDK, kind of an edge case that doesn't bother me much.

I understand your point, but I still think it is good practice to keep a title organized and unified.

asafkorem · 2025-01-07T12:48:28Z

src/integration tests/index.test.ts

+        });
+        it('should call relevant functions to extend the catalog', () => {


missing empty line

asafkorem · 2025-01-07T12:49:18Z

src/types.ts

+
+    /**
+     * Extends the API catalog of the testing framework with additional categories and items.
+     * @param context The variables of the testing framework (i.e. exposes the matching function, expect, etc.).
+     * @param categories The categories to add to the API catalog.
+     * @note This can be used to add custom categories and items to the API catalog.
+     */
+    extendAPICatalog: (categories: TestingFrameworkAPICatalogCategory[] | TestingFrameworkAPICatalogCategory, context?: any,) => void;


categories and items → APIs (categories and JS context)

asafkorem · 2025-01-07T12:52:29Z

src/utils/PromptCreator.test.ts

+
+    describe('extentAPICategories', () => {
+        it('should extend the API catalog with new or exist category', () => {
+            // Add new category


test code should be semantic enough so the comment will be redundant

asafkorem · 2025-01-07T12:55:14Z

src/utils/PromptCreator.ts

+    extendAPICategories(newCategories: TestingFrameworkAPICatalogCategory[] | TestingFrameworkAPICatalogCategory): void {
+        const categoriesArray = Array.isArray(newCategories) ? newCategories : [newCategories];
+        for (const category of categoriesArray) {
+            const existingCategory = this.apiCatalog.categories.find((existingCategory) => existingCategory.title === category.title);
+            if (existingCategory) {
+                for (const item of category.items) {
+                    existingCategory.items.push(item);
+                }
+            } else {
+                this.apiCatalog.categories.push(category);
+            }
+        }
+    }


too many indentations (third-level blocks). let's try to flatten the code:

extendAPICategories( newCategories: TestingFrameworkAPICatalogCategory[] | TestingFrameworkAPICatalogCategory ): void { const categories = Array.isArray(newCategories) ? newCategories : [newCategories]; categories.forEach((category) => { const existingCategory = this.apiCatalog.categories.find( (c) => c.title === category.title ); if (existingCategory) { existingCategory.items.push(...category.items); } else { this.apiCatalog.categories.push(category); } }); }

I think it's better if we won't allow passing a single category as an argument for parameter that is called categories

…ies names in the utils

LironMShemen added 4 commits December 30, 2024 17:14

feat: Add API method

3c80c16

Add the function to types.ts and index.ts

feat: implement the function extendAPICatalog

b7f1329

- Add `extendAPICategories` function to `PromptCreator` - Add `extendJSContext` function to `StepPerformer - Add `extendAPICatalog` function to `Copilot`

chore: expose the function in index.ts and types.ts

4180c3b

test: add suitable test suites.

73e1858

suitable test suites. test: suitable test suites. test: add suitable test suites - Add `Copilot`, `StepPerformer`, `index` and `PromptCreator` suites - Add utils file - Add snapshot file for `PromptCreator` test

LironMShemen force-pushed the Introduce-extendAPICatalog-method branch from a4baefd to 73e1858 Compare January 6, 2025 15:21

asafkorem requested changes Jan 6, 2025

View reviewed changes

LironMShemen added 4 commits January 7, 2025 11:09

fix: typo (extend*)

9f62174

fix:

96c27a0

fix: changes to allow the function to get either a list of categories or a single category object

fix: remove redundant config field

83ca070

fix: change dummyFucntions to jest.fn() and remove redundant copilo…

7366a89

…t test

LironMShemen force-pushed the Introduce-extendAPICatalog-method branch from 82eb204 to 7366a89 Compare January 7, 2025 11:30

asafkorem requested changes Jan 7, 2025

View reviewed changes

LironMShemen added 5 commits January 7, 2025 15:51

fix: change back to function that get only array + change the categor…

5f28957

…ies names in the utils

fix: indentations and empty lines

f435b2c

fix: improve documentation and var name

b718ad1

fix: split suite to 2 tests, to make it more semantic

75bcdf1

chore: save the new snapshot

40166a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce extend api catalog method #34

Introduce extend api catalog method #34

LironMShemen commented Jan 6, 2025

asafkorem left a comment

asafkorem Jan 6, 2025

LironMShemen Jan 7, 2025

asafkorem Jan 6, 2025

asafkorem Jan 6, 2025

asafkorem Jan 6, 2025

LironMShemen Jan 7, 2025

asafkorem Jan 6, 2025

asafkorem Jan 6, 2025

LironMShemen Jan 7, 2025

asafkorem Jan 6, 2025

asafkorem left a comment

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

LironMShemen Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

asafkorem Jan 7, 2025

		});
		it('should call relevant functions to extend the catalog', () => {

Introduce extend api catalog method #34

Are you sure you want to change the base?

Introduce extend api catalog method #34

Conversation

LironMShemen commented Jan 6, 2025

asafkorem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asafkorem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment