fix: Change log-step to a jinja template rather than a simpleeval expression #488
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ression Signed-off-by: Diwank Singh Tomer <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 31e97d4 in 12 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted 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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on df220b4 in 24 seconds
More details
- Looked at
50lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. agents-api/tests/test_execution_workflow.py:407
- Draft comment:
The test forlog_stepis 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 forlog_stepis 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Diwank Singh Tomer [email protected]
Summary:
The PR updates the
log_stepfunction to use Jinja templates for log rendering, replacing simpleeval expressions, and adjusts a test case accordingly.Key points:
log_stepfunction to use Jinja templates for log rendering.Generated with ❤️ by ellipsis.dev