-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: testing differents prompts #23
Conversation
5c99854
to
4762689
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, I have the following suggestions:
- Line 97: The order of concatenation seems to be incorrect. The
instructionJsonFormat
should be appended aftercontentSystemMessage
, not before. Consider changing it to:
- contentSystemMessage = `${contentSystemMessage}\n${instructionJsonFormat}`;
+ contentSystemMessage = `${contentSystemMessage}\n\n${instructionJsonFormat}`;
- Line 110: There is an unnecessary indentation in this line. Remove the extra spaces before
contentSystemMessage
:
- contentSystemMessage = overridePrompt;
+ contentSystemMessage = overridePrompt;
- Line 113: There are two consecutive newlines after
instructionJsonFormat
. Consider removing one of them to maintain consistency:
- contentSystemMessage = `${instructionJsonFormat}\n\n${contentSystemMessage}`;
+ contentSystemMessage = `${instructionJsonFormat}\n${contentSystemMessage}`;
- Line 116: There are two consecutive newlines before
appendPrompt
. Consider removing one of them to maintain consistency:
- contentSystemMessage = `${contentSystemMessage}\n\n${appendPrompt}`;
+ contentSystemMessage = `${contentSystemMessage}\n${appendPrompt}`;
Please review and make the necessary changes accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, I have the following suggestions:
-
Line 144: The temperature value has been changed from 0 to 0.1. Please ensure that this change is intentional and aligns with the desired behavior.
-
Line 157: The indentation of this line seems to be incorrect. Please make sure it is properly aligned with the surrounding code.
-
Line 161: The
max_tokens
value is being parsed as an integer. Please ensure that themaxTokens
variable is a valid integer value before parsing it. -
Line 163: The
frequency_penalty
value is set to 0. Please verify if this value is intended and aligns with the desired behavior.
Please review these suggestions and make any necessary changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, I have the following suggestions:
- Line 169: The variable
response
is accessed without checking if it is defined. It is recommended to add a nullish coalescing operator (??
) to provide a default value in caseresponse
isnull
orundefined
. This will ensure that the code does not throw an error. Here's the updated code:
- const result = response?.trim() || "[]";
+ const result = response?.trim() ?? "[]";
- Line 156: The code is returning
result
without parsing it as JSON. Since the variableresult
is expected to be a JSON string, it should be parsed usingJSON.parse()
before returning. Here's the updated code:
- return result;
+ return JSON.parse(result);
- Line 173: The
console.error()
statement is not necessary and can be removed. Since the error is already caught and handled, logging the error to the console is redundant. Here's the updated code:
- console.error("Error:", error);
- Line 174: The
return null;
statement is not necessary and can be removed. Since the error is caught and handled, returningnull
does not provide any additional information. Here's the updated code:
- return null;
Please consider these suggestions for improving the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, here are my suggestions:
-
Line 182: There is an unnecessary empty line. Please remove it.
-
Line 183: The variable
aiResponses
is being reassigned without any need. Please remove this line. -
Line 185: The arrow function passed to
flatMap
is missing curly braces. Please add curly braces to improve code readability. -
Line 186: The condition
!file.to
seems to be incomplete or incorrect. Please review and update the condition accordingly.
Please make the necessary changes and resubmit the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, I have the following suggestions:
-
Line 177: The function name
createReviewComment
has been changed tocreateReviewOnPr
. Please make sure to update all the references to this function throughout the codebase. -
Line 193: There is an unnecessary empty line. Please remove it to keep the code clean and concise.
-
Line 194: There is an unnecessary empty line. Please remove it to keep the code clean and concise.
-
Line 196: The function
createReviewOnPr
has been added. Please make sure to update all the references to this function throughout the codebase.
Please make the necessary changes based on the suggestions above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, I have the following suggestions:
-
Line 203: The function
createCommentOnPr
is defined but not being used. Consider removing this unused function to keep the codebase clean and avoid confusion. -
Line 207: The
octokit.pulls.createReview
function is called without awaiting its completion. This may lead to unexpected behavior or race conditions. Consider adding theawait
keyword before the function call to ensure proper execution. -
Line 213: The
event
property is set to "COMMENT" in theoctokit.pulls.createReview
function call. However, this property is not supported in the GitHub API. Please review the API documentation and update the event property accordingly. -
Line 215: The
main
function is defined but not being called. Make sure to call themain
function to execute the desired functionality.
Please address these suggestions in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code diff, here are my suggestions:
-
Line 236: The code is calling the
createReviewComment
function, but it seems like it should be callingcreateReviewOnPr
instead. Please update the function name tocreateReviewOnPr
to ensure consistency. -
Line 263: There is a typo in the comment. It says "analyzeCode" instead of "analyzeDiff". Please update the comment to say "analyzeDiff" to accurately reflect the function being called.
-
Line 264: There is an unnecessary empty line. Please remove this empty line to keep the code clean and concise.
-
Line 267: There is a trailing whitespace at the end of the line. Please remove this whitespace to maintain consistent code formatting.
Please make the necessary changes based on the suggestions above.
|
||
for (const file of parsedDiff) { | ||
if (file.to === "/dev/null") continue; // Ignore deleted files | ||
for (const chunk of file.chunks) { | ||
|
||
const messages = createMessages(file, chunk, prDetails); | ||
const messages = generateMessages(file, chunk, prDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name 'generateMessages' does not match the function name used in the code 'createMessages'. Please update the function name to 'createMessages' to maintain consistency.
const newComments = createComment(file, chunk, aiResponse); | ||
if (newComments) { | ||
comments.push(...newComments); | ||
if (!isJSON(aiResponse)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name 'allReviews' does not accurately describe its purpose. Consider renaming it to 'comments' to match the variable used in the code.
if (newComments) { | ||
comments.push(...newComments); | ||
if (!isJSON(aiResponse)) { | ||
logger.log(`AI response is not in JSON format: ${aiResponse}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition '!isJSON(aiResponse)' is not clear. Please add a comment explaining the purpose of this condition.
comments.push(...newComments); | ||
if (!isJSON(aiResponse)) { | ||
logger.log(`AI response is not in JSON format: ${aiResponse}`); | ||
createCommentOnPr(aiResponse, prDetails.owner, prDetails.repo, prDetails.pull_number, file.to); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function 'createCommentOnPr' is called inside the loop. Consider moving it outside the loop to avoid unnecessary function calls.
No description provided.