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

refactor loopblock value #1379

Merged
merged 1 commit into from
Dec 12, 2024
Merged

refactor loopblock value #1379

merged 1 commit into from
Dec 12, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Dec 12, 2024

Important

Refactor loop block value handling by introducing loop_variable_reference and updating related logic in ForLoopBlock, YAML, and service components.

  • Exceptions:
    • Add FailedToFormatJinjaStyleParameter and NoIterableValueFound in exceptions.py.
  • ForLoopBlock:
    • Add loop_variable_reference attribute in block.py.
    • Modify get_loop_over_parameter_values() to handle loop_variable_reference.
    • Update execute() to handle exceptions from get_loop_over_parameter_values().
  • YAML:
    • Add loop_variable_reference to ForLoopBlockYAML in yaml.py.
  • Service:
    • Update block_yaml_to_block() in service.py to handle loop_variable_reference.

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

@LawyZheng LawyZheng merged commit 0a6a2d8 into main Dec 12, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/refactor-loopblock-value branch December 12, 2024 17:19
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. Reviewed everything up to d8735c2 in 13 minutes and 23 seconds

More details
  • Looked at 199 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/block.py:754
  • Draft comment:
    Consider including the exception message in the failure reason to provide more context on the error.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_e8ITv9lbVZwe5xw0


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.

)
except Exception as e:
raise FailedToFormatJinjaStyleParameter(value_template, str(e))
parameter_value = json.loads(value_json)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding exception handling for json.loads to manage potential JSON parsing errors gracefully.

loop_over_parameter = parameters[trimmed_key]

if loop_over_parameter is None and not block_yaml.loop_variable_reference:
raise Exception("empty loop value parameter")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider raising a more specific exception instead of a generic Exception for better error handling.

@LawyZheng LawyZheng restored the lawy/refactor-loopblock-value branch December 12, 2024 18:08
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.

1 participant