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: Change log-step to a jinja template rather than a simpleeval expression #488

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Sep 3, 2024

Signed-off-by: Diwank Singh Tomer diwank.singh@gmail.com


🚀 This description was created by Ellipsis for commit df220b4

Summary:

The PR updates the log_step function to use Jinja templates for log rendering, replacing simpleeval expressions, and adjusts a test case accordingly.

Key points:

  • Update log_step function to use Jinja templates for log rendering.
  • Replace simpleeval expressions with Jinja templates.
  • Modify test case to use Jinja syntax for log expressions.

Generated with ❤️ by ellipsis.dev

…ression

Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
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 31e97d4 in 12 seconds

More details
  • Looked at 49 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. agents-api/tests/test_execution_workflow.py:425
  • Draft comment:
    The test for log step expression failure is commented out. Consider re-enabling it to ensure the new Jinja template logic is tested for failure cases.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is about a change made in the diff, which is the commenting out of a test assertion. The comment suggests re-enabling the test to ensure the new logic is tested for failure cases, which is a valid suggestion for code quality and testing coverage.
    The comment could be seen as speculative since it suggests re-enabling a test without knowing the reason it was commented out. However, it is actionable and related to a change in the diff.
    The comment is actionable because it suggests a specific change (re-enabling the test) that would improve testing coverage. It is related to a change in the diff, so it is relevant.
    The comment should be kept as it is related to a change in the diff and suggests an actionable improvement to the code.

Workflow ID: wflow_R5BvNeRYmTnsNArW


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 df220b4 in 24 seconds

More details
  • Looked at 50 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. agents-api/tests/test_execution_workflow.py:407
  • Draft comment:
    The test for log_step is commented out. Consider uncommenting and updating it to ensure the new Jinja template functionality is tested.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is not about a change made in the diff. It refers to a test that is commented out, but the diff does not show any changes to that test. Therefore, the comment should be removed as it does not pertain to the changes made in this PR.
    I might be missing the context of why the comment was made, but based on the rules, it should be removed because it does not relate to a change in the diff.
    The rules are clear that comments should only be made on changes in the diff. Since this comment does not pertain to a change, it should be removed.
    Remove the comment because it does not relate to a change made in the diff.
2. agents-api/tests/test_execution_workflow.py:424
  • Draft comment:
    The test for log_step is commented out. Consider uncommenting and updating it to ensure the new Jinja template functionality is tested.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_jzGHi5lDnBkSXaNa


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

@HamadaSalhab HamadaSalhab merged commit 6fe6e4e into dev-tasks Sep 4, 2024
2 of 5 checks passed
@HamadaSalhab HamadaSalhab deleted the x/log-step branch September 4, 2024 23:12
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.

2 participants