Closed sohamganatra closed 1 month ago
Your free trial has expired. To keep using Ellipsis, sign up at https://app.ellipsis.dev for $20/seat/month or reach us at help@ellipsis.dev
โฑ๏ธ Estimated effort to review: 2 ๐ต๐ตโชโชโช |
๐งช No relevant tests |
๐ No security concerns identified |
โก Key issues to review Code Consistency The use of `exclude_none=True` has been added to ensure `None` values are not serialized. It's important to verify that this change aligns with all use cases where `_serialize_execute_params` is used, as it might affect the output where `None` values were previously expected. |
Category | Suggestion | Score |
Possible issue |
Add exception handling for serialization methods when excluding None values___ **Consider handling the potential exceptions that might be raised bymodel_dump_json() and dict() methods when exclude_none=True is used. This is especially important if the serialization process might encounter data types that do not support exclusion of None values directly.** [python/composio/tools/toolset.py [292-295]](https://github.com/ComposioHQ/composio/pull/343/files#diff-89b5c0aec15f01eb54ad26f78731b68f650e438a443378282b436df85abde51dR292-R295) ```diff -return param.model_dump_json(exclude_none=True) # type: ignore -return param.dict(exclude_none=True) # type: ignore +try: + return param.model_dump_json(exclude_none=True) # type: ignore +except Exception as e: + handle_exception(e) +try: + return param.dict(exclude_none=True) # type: ignore +except Exception as e: + handle_exception(e) + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: The suggestion correctly identifies a potential issue with the `model_dump_json()` and `dict()` methods when `exclude_none=True` is used. Adding exception handling improves the robustness of the code by ensuring that any unexpected errors during serialization are properly managed. | 8 |
**Action:** test (ubuntu-latest, 3.10) |
**Failed stage:** [Unittests](https://github.com/ComposioHQ/composio/actions/runs/10007813098/job/27663333613) [โ] |
**Failed test name:** test_action_enum |
**Failure summary:**
The action failed because the test test_action_enum failed. The test failed due to a ValueError being raised in the Action class's __init__ method. The error message indicated that the value GITHUB_ISSUES_LIST is not a valid value for the Action enum. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 506: * [new branch] fix/readme -> origin/fix/readme 507: * [new branch] fix/readme-logo -> origin/fix/readme-logo 508: * [new branch] fix/swe-agent -> origin/fix/swe-agent 509: * [new branch] ft-add-better-help-text -> origin/ft-add-better-help-text 510: * [new branch] ft-apps-id -> origin/ft-apps-id 511: * [new branch] ft-bring-back-core-sdk -> origin/ft-bring-back-core-sdk 512: * [new branch] ft-custom-base-url-js-sdk -> origin/ft-custom-base-url-js-sdk 513: * [new branch] ft-did-you-mean -> origin/ft-did-you-mean 514: * [new branch] ft-error-tracking -> origin/ft-error-tracking ... 929: tests/test_cli/test_actions.py::TestListActions::test_list_all[arguments3-exptected_outputs3-unexptected_outputs3] PASSED [ 17%] 930: tests/test_cli/test_actions.py::TestListActions::test_tag_not_found PASSED [ 19%] 931: tests/test_cli/test_actions.py::TestListActions::test_limit SKIPPED [ 21%] 932: tests/test_cli/test_actions.py::TestListActions::test_copy PASSED [ 23%] 933: tests/test_cli/test_add.py::TestComposioAdd::test_no_auth PASSED [ 25%] 934: tests/test_cli/test_apps.py::TestList::test_list PASSED [ 27%] 935: tests/test_cli/test_apps.py::TestUpdate::test_update_not_required PASSED [ 29%] 936: tests/test_cli/test_apps.py::TestUpdate::test_update SKIPPED (Needs 937: investigation, this test fails in CI) [ 31%] ... 945: tests/test_client/test_client.py::test_raise_invalid_api_key PASSED [ 48%] 946: tests/test_client/test_collections.py::TestTriggerNamesSerialization::test_converts_trigger_objects_to_comma_separated_string PASSED [ 51%] 947: tests/test_client/test_collections.py::TestTriggerNamesSerialization::test_converts_trigger_strings_to_comma_separated_string PASSED [ 53%] 948: tests/test_client/test_collections.py::TestTriggerNamesSerialization::test_converts_mix_of_trigger_objects_and_strings PASSED [ 55%] 949: tests/test_client/test_collections.py::test_trigger_subscription PASSED [ 57%] 950: tests/test_client/test_endpoints.py::test_endpoint PASSED [ 59%] 951: tests/test_client/test_enum.py::test_tag_enum PASSED [ 61%] 952: tests/test_client/test_enum.py::test_app_enum PASSED [ 63%] 953: tests/test_client/test_enum.py::test_action_enum FAILED [ 65%] ... 962: tests/test_tools/test_local/test_workspace.py::test_stderr PASSED [ 85%] 963: tests/test_tools/test_local/test_workspace.py::test_workspace PASSED [ 87%] 964: tests/test_utils/test_decorators.py::test_deprecated PASSED [ 89%] 965: tests/test_utils/test_git.py::test_get_git_user_info PASSED [ 91%] 966: tests/test_utils/test_shared.py::test_get_pydantic_signature_format_from_schema_params PASSED [ 93%] 967: tests/test_utils/test_shared.py::test_json_schema_to_pydantic_field PASSED [ 95%] 968: tests/test_utils/test_shared.py::test_json_schema_to_fields_dict PASSED [ 97%] 969: tests/test_utils/test_url.py::test_get_web_url PASSED [100%] 970: =================================== FAILURES =================================== ... 980: self = |
PR Type
Bug fix
Description
_serialize_execute_params
method whereNone
values were not being excluded during JSON serialization.model_dump_json
anddict
method calls to includeexclude_none=True
parameter.Changes walkthrough ๐
toolset.py
Exclude `None` values in JSON serialization for models
python/composio/tools/toolset.py
_serialize_execute_params
method to excludeNone
values inJSON serialization.
model_dump_json
anddict
methods to useexclude_none=True
.