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 default value DB mapping #857

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Fix default value DB mapping #857

merged 1 commit into from
Sep 19, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Sep 19, 2024

[!IMPORTANT].
Serialize default_value to JSON in create_workflow_parameter in client.py..
.

  • Serialize default_value to JSON in create_workflow_parameter in client.py..
  • Behavior:.
    • In create_workflow_parameter function in client.py, default_value is now serialized to JSON using json.dumps() before being stored in the database..
      .
      This description was created by Ellipsis for 9ddcf76. It will automatically update as commits are pushed.

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT].
> Serialize `default_value` to JSON in `create_workflow_parameter` in `client.py`..
> .
>   - **Behavior**:.
>     - In `create_workflow_parameter` function in `client.py`, `default_value` is now serialized to JSON using `json.dumps()` before being stored in the database..
> .
> <sup>This description was created by </sup><img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173&link=https%3A%2F%2Fellipsis.dev"><sup> for a1291022ebf8681a33801f12f279e20993e23727. It will automatically update as commits are pushed.</sup>

<!-- 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 9ddcf76 in 22 seconds

More details
  • Looked at 18 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:1114
  • Draft comment:
    Ensure default_value is JSON serializable before using json.dumps() to avoid potential TypeError.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative, as it suggests a potential issue without strong evidence that it will occur. The use of json.dumps() inherently requires the input to be JSON serializable, and this is a known requirement of the function. The comment does not provide a specific code change or improvement, but rather a cautionary note.
    I might be underestimating the potential for default_value to be non-serializable, but the comment does not suggest a specific code change to address this. It is more of a reminder than an actionable comment.
    The comment does not suggest a specific code change or improvement, and the requirement for JSON serializability is inherent to the use of json.dumps().
    The comment should be deleted as it is speculative and does not suggest a specific code change. The requirement for JSON serializability is inherent to the use of json.dumps().

Workflow ID: wflow_uuN7NUSfOmBP11vQ


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.

❌ Changes requested. Incremental review on 9ddcf76 in 29 seconds

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

Workflow ID: wflow_PDWKHe6dUwYNxnAh


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

skyvern/forge/sdk/db/client.py Show resolved Hide resolved
@ykeremy ykeremy merged commit 1f282e7 into main Sep 19, 2024
2 checks passed
@ykeremy ykeremy deleted the ykeremy/fix-default-value branch September 19, 2024 18:09
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