[code_review] Add explanation to the generated comments #5005
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an explanation property to generated comments and updates related documentation and examples to include justification information in the JSON objects.
- Added an explanation attribute to the InlineComment class.
- Adjusted documentation and JSON examples to require an explanation key in each comment.
- Updated the generate_processed_output and _get_comment_examples functions to propagate the explanation field.
| @@ -51,6 +51,7 @@ class InlineComment: | |||
| hunk_start_line: int | None = None | |||
| hunk_end_line: int | None = None | |||
| is_generated: bool | None = None | |||
| explanation: str | None = None | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
[nitpick] Consider adding a docstring or inline comment to clarify the purpose of the 'explanation' attribute in InlineComment.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
That will not be consistent with the rest of the attributes.
| @@ -1401,6 +1407,9 @@ def _get_comment_examples(self, patch, created_before: datetime | None = None): | |||
|
|
|||
| if not comment_examples: | |||
| comment_examples = STATIC_COMMENT_EXAMPLES | |||
| else: | |||
| for example in comment_examples: | |||
| example["comment"]["explanation"] = "THE JUSTIFICATION GOES HERE" | |||
There was a problem hiding this comment.
In _get_comment_examples, the 'explanation' field is being nested under the 'comment' key; for consistency with the rest of the schema, consider adding 'explanation' as a top-level key in the JSON objects.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Invalid suggestion. The explanation needs to be part of the comment dict.
|
The evaluation results: This can be compared to #5002 (comment). The results look similar, with a minor improvement. |
This comment was marked as off-topic.
This comment was marked as off-topic.
| @@ -152,7 +153,7 @@ class LargeDiffError(Exception): | |||
| - include praising; | |||
| - ask if changes are intentional or ask to ensure things exist. | |||
|
|
|||
| Do not report any explanation about your choice. Only return a valid JSON list. | |||
| Only return a valid JSON list. Do not drop any key from the JSON objects. | |||
There was a problem hiding this comment.
Is the second part needed?
There was a problem hiding this comment.
At the beginning, it was without the sound part. However, the filtering step was omitting the explanation key from he final result.
| @@ -212,6 +214,7 @@ class LargeDiffError(Exception): | |||
| "filename": "netwerk/streamconv/converters/mozTXTToHTMLConv.cpp", | |||
| "start_line": 1211, | |||
| "content": "You are using `nsAutoStringN<256>` instead of `nsString`. This is a good change as `nsAutoStringN<256>` is more efficient for small strings. However, you should ensure that the size of `tempString` does not exceed 256 characters, as `nsAutoStringN<256>` has a fixed size.", | |||
| "explanation": "THE JUSTIFICATION GOES HERE", | |||
There was a problem hiding this comment.
The generated explanation seems reasonable, even without it.
| @@ -1401,6 +1407,9 @@ def _get_comment_examples(self, patch, created_before: datetime | None = None): | |||
|
|
|||
| if not comment_examples: | |||
| comment_examples = STATIC_COMMENT_EXAMPLES | |||
| else: | |||
| for example in comment_examples: | |||
| example["comment"]["explanation"] = "THE JUSTIFICATION GOES HERE" | |||
There was a problem hiding this comment.
Here we should reuse the old generated explanations when there are some
There was a problem hiding this comment.
I'm not sure that would add value, especially since it will not be consistent when some comments have justification value and others do not. But it will definitely consume more tokens.
I added the placeholder here to avoid situations where the explanation is omitted.
Resolves #4785