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.

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

feat: move to object params for methods + processor support #966

Merged
merged 52 commits into from
Dec 9, 2024

Conversation

himanshu-dixit
Copy link
Collaborator

@himanshu-dixit himanshu-dixit commented Dec 9, 2024

Important

Refactor code to use destructured parameters and Zod validation, update tests, and improve logging and error handling.

  • Refactoring:
    • Destructure parameters in executeAction in base.toolset.ts and execute in Entity.ts.
    • Use Zod for parameter validation in Entity.ts for execute and initiateConnection.
    • Update method signatures to use destructured objects in getConnection and setupTrigger in Entity.ts.
  • Tests:
    • Update tests in index.spec.ts, Entity.spec.ts, apps.spec.ts, and triggers.spec.ts to match new method signatures.
  • Misc:
    • Remove unused imports in base.toolset.ts.
    • Minor updates to logging and error handling across files.

This description was created by Ellipsis for 1f85150. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 2629a82 in 22 seconds

More details
  • Looked at 249 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. js/src/sdk/base.toolset.ts:41
  • Draft comment:
    Avoid using @ts-expect-error to bypass TypeScript checks. Ensure toolResponse.data.response_data is properly typed to avoid errors.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. js/src/sdk/base.toolset.ts:55
  • Draft comment:
    Avoid using @ts-ignore to bypass TypeScript checks. Ensure toolResponse.data.response_data is properly typed to avoid errors.
  • Reason this comment was not posted:
    Marked as duplicate.
3. js/src/sdk/base.toolset.ts:312
  • Draft comment:
    Avoid using @ts-ignore to bypass TypeScript checks. Ensure data.response_data is properly typed to avoid errors.
  • Reason this comment was not posted:
    Marked as duplicate.
4. js/src/sdk/base.toolset.ts:337
  • Draft comment:
    Consider making the error message more descriptive to indicate which processor type is invalid.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addPreProcessor and addPostProcessor methods throw an error if the processor is not a function. This is a good practice for type safety. However, the error message could be more descriptive to indicate which processor type is invalid.

Workflow ID: wflow_AsLBiLcF9rGpPfd0


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on f19cac4 in 16 seconds

More details
  • Looked at 77 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/src/sdk/base.toolset.ts:73
  • Draft comment:
    Consider renaming the parameter processor to postProcessor in the addPostProcessor method for consistency with addPreProcessor.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The type alias TProcessor was renamed to TPreProcessor in the code, but the type alias TPostProcessor was not updated in the addPostProcessor method. This inconsistency should be addressed for clarity and consistency.

Workflow ID: wflow_3tAy8DclxTCFdqCj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -60,12 +108,6 @@ export class ComposioToolSet {

}

async getExpectedParamsForUser(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep it here, if anything let's deprecate it and remove it sometime in the future to avoid breaking running code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Base automatically changed from ft-handle-other-400-errors to master December 9, 2024 10:32
@himanshu-dixit himanshu-dixit changed the base branch from master to sawra/file_upload_refactor December 9, 2024 12:27
@himanshu-dixit himanshu-dixit changed the base branch from sawra/file_upload_refactor to master December 9, 2024 12:27
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 1f85150 in 11 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_97G4xtDl00TenWRE


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@himanshu-dixit himanshu-dixit merged commit 1483573 into master Dec 9, 2024
11 checks passed
@himanshu-dixit himanshu-dixit deleted the ft-add-zod-checks branch December 9, 2024 12:46
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.

3 participants