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

fix: propagate default values from trigger config #970

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Conversation

tushar-composio
Copy link
Contributor

@tushar-composio tushar-composio commented Dec 9, 2024

the TriggerModel's that are defined for triggers in the backend have properties, and each property may have a default field which we were ignoring in the SDK. Now we store that in the TriggerConfig object, and also add the default values into the config that we send in the request to activate a new trigger.


Important

Propagate default values from trigger configurations to ensure correct trigger activation in the SDK.

  • Behavior:
    • Populate default values for optional fields in config in _enable_trigger() in triggers.py.
    • Include default values in trigger activation request.
  • Models:
    • Add default field to TriggerConfigPropertyModel in collections.py.

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

Copy link

vercel bot commented Dec 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 11:19am

@shreysingla11
Copy link
Collaborator

Code Review Summary

The changes look good overall and implement a useful feature for handling default values in trigger configurations. The implementation is clean and integrates well with the existing codebase.

Strengths:

✅ Clean implementation of default value propagation
✅ Maintains backward compatibility
✅ Good separation of concerns between model and trigger handling

Suggestions for Improvement:

  1. Consider adding type validation for default values
  2. Add explicit validation for required fields
  3. Consider adding tests for the new functionality

Overall Rating: 8/10 - Solid implementation with room for minor improvements in validation and testing.

Copy link

github-actions bot commented Dec 9, 2024

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-12234539849/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-12234539849/html-report/report.html

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! Reviewed everything up to aa7e1c1 in 1 minute and 46 seconds

More details
  • Looked at 33 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/client/collections.py:389
  • Draft comment:
    Consider changing the type of default to Optional[t.Any] to indicate that it can be None. This aligns with its usage in triggers.py.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addition of the default attribute in TriggerConfigPropertyModel is appropriate and aligns with the changes in triggers.py. However, the type should be Optional[t.Any] to indicate that it can be None. This change should be consistent with the usage in triggers.py. The same applies to other similar models like AuthSchemeField and ExpectedFieldInput.

Workflow ID: wflow_jM4nCVygFr5opxKt


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 d119692 in 13 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/client/collections.py:389
  • Draft comment:
    Setting a default value for default in TriggerConfigPropertyModel is a good practice to avoid uninitialized fields.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change made in the PR is to set a default value for the default field in TriggerConfigPropertyModel. This is a good practice to avoid potential issues with uninitialized fields.

Workflow ID: wflow_8Hi4IVmSzRfEDpJs


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

@tushar-composio tushar-composio merged commit 5c2065f into master Dec 9, 2024
11 checks passed
@tushar-composio tushar-composio deleted the ENG-2849 branch December 9, 2024 11:33
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