Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor tool function calling code #193

Merged
merged 14 commits into from
Mar 26, 2024
Merged

Refactor tool function calling code #193

merged 14 commits into from
Mar 26, 2024

Conversation

barnettwilliam
Copy link
Collaborator

@barnettwilliam barnettwilliam commented Mar 12, 2024 •
edited
Loading

Long awaited changes for #40 refactor tool function calling code. Mostly moving tool function and conversion function invocation to ToolsManager and FunctionRegistry.

There is still moving the getActionFunction() from ToolsManager into FunctionRegistry todo but raised as issue #192 because it requires some rework of FunctionRegistry

@barnettwilliam barnettwilliam linked an issue Mar 12, 2024 that may be closed by this pull request
Refactor function calling code by moving it into ToolManager or FunctionRegistry #40
Closed
szschaler
szschaler requested changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@szschaler szschaler left a comment

Choose a reason for hiding this comment

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

I would feel better about this refactoring if we first had a robust set of tests around calling action functions and conversions using the current implementation before we move the code (and its corresponding, but not established) tests,

@barnettwilliam
Copy link
Collaborator Author

Ok, I can write tests for the existing implementation and then use them here to verify these changes

@barnettwilliam barnettwilliam marked this pull request as draft March 12, 2024 11:16
@barnettwilliam
Moved tool functions and conversion function invocation to ToolsManag…
8935ef2 
…er and FunctionRegistry as part of #40
@barnettwilliam
Fixed unit tests, updated renamed funtions
9fc5070
@barnettwilliam
Updated getActionFunction deprecated issue number
dfea91c
@barnettwilliam
Moved call() test to FunctionRegistry and updated references.
3d7af4a
@barnettwilliam
Moved callConversion() test to FunctionRegistry and updated references.
34d37cd
@barnettwilliam
Reinstated moved functions recent changes that were lost in re-base: 4…
19c31db 
…5e7287 Added defensive check for selectConversionFunctionConvertMetamodel(), cbd180f Modified invokeActionFunction() to not populate unused parameters as 'undefined', and 061e78c corrected typos in comments.
@barnettwilliam
Moved invokeActionFunction() test to ToolManager and updated references.
4831a3b
@barnettwilliam
Moved selectConversionFunctionConvertMetamodel() test to ToolManager …
7f115b6 
…and updated references.
@barnettwilliam
Moved convert() test to ToolManager and updated references.
62439b9
@barnettwilliam
Moved convertIncludingMetamodel() test to ToolManager and updated ref…
cfdfaba 
…erences.
@barnettwilliam
Updated runAction() test references to re-located functions.
313b3ad
@barnettwilliam
Updated test comments to reflect the relocated functions.
722bd10
@barnettwilliam barnettwilliam force-pushed the fix/40-refactor-tool-function-calling-code branch from e44200f to 722bd10 Compare March 22, 2024 14:05
@barnettwilliam
Copy link
Collaborator Author

Rebased to include the tests from PR #201 and updated tests for the relocated functions.

@barnettwilliam barnettwilliam marked this pull request as ready for review March 22, 2024 15:08
szschaler
szschaler approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@szschaler szschaler left a comment

Choose a reason for hiding this comment

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

Some small comments, but overall LGTM

platform/src/EducationPlatformApp.js Show resolved Hide resolved
platform/src/ToolsManager.js Outdated Show resolved Hide resolved
platform/src/ToolsManager.js Outdated Show resolved Hide resolved
platform/test/spec/testFunctionRegistrySpec.js Outdated Show resolved Hide resolved
barnettwilliam and others added 2 commits March 22, 2024 16:49
@barnettwilliam barnettwilliam mentioned this pull request Mar 22, 2024
Merged
@barnettwilliam barnettwilliam merged commit a2de865 into main Mar 26, 2024
7 checks passed
@barnettwilliam barnettwilliam deleted the fix/40-refactor-tool-function-calling-code branch March 26, 2024 09:26
barnettwilliam added a commit that referenced this pull request Mar 28, 2024
@barnettwilliam
Removed convert(), moved in #193
82cd623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szschaler szschaler szschaler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor function calling code by moving it into ToolManager or FunctionRegistry
2 participants
@barnettwilliam @szschaler