enhan: Make result message more readable by json encoding it #254
Conversation
This file contains hidden or 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
AkashS0510
requested review from
Akshat0694,
arunim2405 and
refeed
as code owners
refeed
reviewed
May 13, 2025
Member
refeed
left a comment
refeed
left a comment
There was a problem hiding this comment.
Looks good overall, would you please post a screenshot of what the results look like now?
refeed
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a json_format_value helper to wrap evaluation values in JSON-style backticks, updates all evaluator modules to use it for consistent, readable messages, and adjusts tests to match the new formatting.
- Added
json_format_valueintirith/utils.pyfor JSON-string formatting of values. - Updated each evaluator in
src/tirith/core/evaluators/to calljson_format_valuewhen building their result messages. - Modified tests in
tests/core/evaluators/to assert messages now include JSON formatting/backticks.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tirith/utils.py | Adds json_format_value utility for formatting values. |
| src/tirith/core/evaluators/* | Refactors message strings to use json_format_value. |
| tests/core/evaluators/* | Updates test assertions to expect JSON-formatted values. |
Comments suppressed due to low confidence (2)
tests/core/evaluators/test_is_not_empty.py:20
- The second test function is named
test_evaluate_passingbut is marked as failing. Rename it totest_evaluate_failingto avoid duplicate test names.
def test_evaluate_passing():
src/tirith/utils.py:8
- Consider adding unit tests for
json_format_value, including cases for non-serializable objects to verify the fallback path.
def json_format_value(value: Any) -> str:
|
refeed
approved these changes
May 13, 2025
arunim2405
approved these changes
May 13, 2025
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.
Pull Request Template
This pull request enhances the evaluator modules by introducing the
json_format_valueutility function to improve the readability of evaluation messages and adds missing imports for logging and type hints where necessary. These changes ensure that the evaluation results are more user-friendly and consistent across all evaluators.Improvements to evaluation messages:
json_format_valuefor formatting values in evaluation messages, improving the readability of outputs. This change affects evaluators such ascontained_in,contains,equals,greater_than,greater_than_equal_to,is_empty,is_not_empty,less_than,less_than_equal_to, andnot_contained_in.Code consistency and maintainability:
Description
What changes are being made?
Why are these changes necessary?
Which issues or tickets does this PR close or relate to?
Type of Change
Checklist
Screenshots or Recordings (if applicable)
Additional Information