-
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
Code review #15
Code review #15
Conversation
@@ -0,0 +1,29 @@ | |||
name: Code Reviewer |
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 'name' field is not necessary in this workflow. It can be removed.
@@ -0,0 +1,29 @@ | |||
name: Code Reviewer | |||
run-name: Action started by ${{ github.actor }} |
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 'run-name' field is not a valid field in a workflow. It should be removed.
- opened | ||
- synchronize | ||
|
||
permissions: write-all |
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 'permissions' field is not necessary in this workflow. It can be removed.
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. | ||
- Do not suggest imports issues. |
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.
There is a missing newline at the end of the file. Please add a newline.
@@ -0,0 +1,246 @@ | |||
require('child_process') |
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.
Missing 'const' keyword before require statement.
const core = require('@actions/core'); | ||
const github = require('@actions/github'); | ||
const fs = require('fs'); | ||
const SimpleChatGptService = require('../services/simple_chat_gpt_service.js'); |
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.
Missing 'const' keyword before require statement.
const github = require('@actions/github'); | ||
const fs = require('fs'); | ||
const SimpleChatGptService = require('../services/simple_chat_gpt_service.js'); | ||
const ChatCompletionParams = require('../services/gpt/chat_completion_params.js'); |
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.
Missing 'const' keyword before require statement.
@@ -29,6 +29,18 @@ class SimpleChatGptService { | |||
throw error; | |||
} | |||
} | |||
|
|||
// chatCompletionParams: ChatCompletionParams |
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.
Consider adding a comment to describe the purpose of this method.
// chatCompletionParams: ChatCompletionParams | ||
async fromParams({ chatCompletionParams }) { | ||
try { | ||
const response = await this.service.chatCompletions(chatCompletionParams); |
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.
Consider adding error handling for cases where response.choices is empty.
console.error('[SimpleChatGptService]', error); | ||
throw error; | ||
} | ||
} | ||
} | ||
|
||
module.exports = SimpleChatGptService; |
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.
Consider adding a newline at the end of the file.
ed02d9e
to
f89bfdd
Compare
max_tokens: 900 | ||
exclude: "**/*.json, **/*.md, **/*.g.dart" | ||
append_prompt: | | ||
- Give a maximum of 4 suggestions |
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 line number in the diff is incorrect. It should be line 28 instead of line 27.
exclude: "**/*.json, **/*.md, **/*.g.dart" | ||
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. |
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 comment in the 'append_prompt' should be updated to reflect the correct number of suggestions.
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. | ||
- Do not suggest imports issues. |
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 comment in the 'append_prompt' should be updated to reflect the correct restrictions on code formatting issues.
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. | ||
- Do not suggest imports issues. |
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 comment in the 'append_prompt' should be updated to reflect the correct restrictions on imports issues.
code-review/action.js
Outdated
for (const file of parsedDiff) { | ||
if (file.to === "/dev/null") continue; // Ignore deleted files | ||
for (const chunk of file.chunks) { | ||
logger.log(`Analyzing chunk: ${chunk.changes}\n------------------`); |
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 log message should be more descriptive and provide useful information about the chunk being analyzed.
} | ||
|
||
function createMessages(file, chunk, prDetails) { | ||
const instructionJsonFormat = `- Always provide the response in following JSON format: [{"lineNumber": <line_number>, "reviewComment": "<review comment>"}]`; |
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 'instructionJsonFormat' is defined but not used.
function createMessages(file, chunk, prDetails) { | ||
const instructionJsonFormat = `- Always provide the response in following JSON format: [{"lineNumber": <line_number>, "reviewComment": "<review comment>"}]`; | ||
|
||
var contentSystemMessage = `You are a senior software engineer and your task is to review pull requests for possible bugs or bad development practices. Follow the instructions below: |
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 'contentSystemMessage' is defined but not used.
|
||
var contentSystemMessage = `You are a senior software engineer and your task is to review pull requests for possible bugs or bad development practices. Follow the instructions below: | ||
- You will provide suggestions only if there is possible issues or bugs in the code, otherwise return an empty array. | ||
- Do not give positive comments or compliments. |
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 '+' sign is not needed here.
code-review/action.js
Outdated
var contentSystemMessage = `You are a senior software engineer and your task is to review pull requests for possible bugs or bad development practices. Follow the instructions below: | ||
- You will provide suggestions only if there is possible issues or bugs in the code, otherwise return an empty array. | ||
- Do not give positive comments or compliments. | ||
- Don't suggest removing empty line or adding line to end of file |
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 '-' sign is not needed here.
b27bb48
to
48b859f
Compare
max_tokens: 900 | ||
exclude: "**/*.json, **/*.md, **/*.g.dart" | ||
append_prompt: | | ||
- Give a maximum of 4 suggestions |
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 line number in the diff is incorrect. It should be line 28 instead of line 27.
exclude: "**/*.json, **/*.md, **/*.g.dart" | ||
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. |
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 comment in the 'append_prompt' should be updated to reflect the correct number of suggestions.
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. | ||
- Do not suggest imports issues. |
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 comment in the 'append_prompt' should be updated to reflect the correct instructions about code formatting issues.
append_prompt: | | ||
- Give a maximum of 4 suggestions | ||
- Do not suggest code formatting issues. | ||
- Do not suggest imports issues. |
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 comment in the 'append_prompt' should be updated to reflect the correct instructions about imports issues.
for (const file of parsedDiff) { | ||
if (file.to === "/dev/null") continue; // Ignore deleted files | ||
for (const chunk of file.chunks) { | ||
|
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.
There is an extra empty line at line 65.
} | ||
|
||
function createMessages(file, chunk, prDetails) { | ||
const instructionJsonFormat = `- Always provide the response in following JSON format: [{"lineNumber": <line_number>, "reviewComment": "<review comment>"}]`; |
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 instructionJsonFormat
is defined but not used.
function createMessages(file, chunk, prDetails) { | ||
const instructionJsonFormat = `- Always provide the response in following JSON format: [{"lineNumber": <line_number>, "reviewComment": "<review comment>"}]`; | ||
|
||
var contentSystemMessage = `You are a senior software engineer and your task is to review pull requests for possible bugs or bad development practices. Follow the instructions below: |
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 contentSystemMessage
is defined but not used.
|
||
var contentSystemMessage = `You are a senior software engineer and your task is to review pull requests for possible bugs or bad development practices. Follow the instructions below: | ||
- You will provide suggestions only if there is possible issues or bugs in the code, otherwise return an empty array. | ||
- Do not give positive comments or compliments. |
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 instruction 'Do not give positive comments or compliments.' is duplicated.
var contentSystemMessage = `You are a senior software engineer and your task is to review pull requests for possible bugs or bad development practices. Follow the instructions below: | ||
- You will provide suggestions only if there is possible issues or bugs in the code, otherwise return an empty array. | ||
- Do not give positive comments or compliments. | ||
- Don't suggest removing empty line or adding line to end of file. |
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 instruction 'Don't suggest removing empty line or adding line to end of file.' is duplicated.
48b859f
to
9df47c2
Compare
No description provided.