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

Only use json.dumps when workflow parameter type is json #859

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Sep 19, 2024

<!-- ELLIPSIS_HIDDEN -->
🚀 This description was created by 509f882

fix: conditionally apply json.dumps for JSON type parameters in create_workflow_parameter

Summary:

Conditionally apply json.dumps in create_workflow_parameter() for JSON type parameters only.

Key points:

  • Conditionally apply json.dumps in create_workflow_parameter() for JSON type parameters.
  • json.dumps is applied only when workflow_parameter_type is WorkflowParameterType.JSON.
  • For non-JSON types, default_value is used as-is without serialization.

Generated with ❤️ by ellipsis.dev

<!-- ELLIPSIS_HIDDEN -->

| 🚀 | This description was created by c29b98905b2151c758165c90cacbfcd50129ea83  |
|--------|--------|

fix: conditionally apply json.dumps for JSON type parameters in create_workflow_parameter

### Summary:
Conditionally apply `json.dumps` in `create_workflow_parameter()` for JSON type parameters in `client.py`.

**Key points**:
- **Behavior**:
  - In `create_workflow_parameter()` in `client.py`, `json.dumps` is now conditionally applied only when `workflow_parameter_type` is `WorkflowParameterType.JSON`.
  - For non-JSON types, `default_value` is used as-is without serialization.

----
Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev)

<!-- ELLIPSIS_HIDDEN -->
@ykeremy ykeremy added the sync label Sep 19, 2024
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 509f882 in 10 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:1127
  • Draft comment:
    Ensure that WorkflowParameterType.JSON is correctly defined and used throughout the codebase to avoid potential issues with type checking.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR description indicates that json.dumps should only be applied when the parameter type is JSON. The current implementation correctly checks for this condition before applying json.dumps. However, it's important to ensure that the WorkflowParameterType.JSON is correctly defined and used in the codebase.

Workflow ID: wflow_Ezu4xIrVCKwHxv1u


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 509f882 in 12 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:1126
  • Draft comment:
    Consider using a ternary operator for better readability:
                default_value = json.dumps(default_value) if workflow_parameter_type == WorkflowParameterType.JSON else default_value
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The PR changes the serialization of default_value to be conditional based on the workflow_parameter_type. This is a good change as it prevents unnecessary serialization for non-JSON types. However, the code can be slightly optimized for readability.

Workflow ID: wflow_qcX6astDPbw9J7EO


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

@ykeremy ykeremy merged commit 30922a3 into main Sep 19, 2024
2 checks passed
@ykeremy ykeremy deleted the ykeremy/fix-default-value-2 branch September 19, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant