Skip to content
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 #16

Closed
wants to merge 6 commits into from
Closed

Code review #16

wants to merge 6 commits into from

Conversation

emanuel-braz
Copy link
Owner

No description provided.

@@ -0,0 +1,29 @@
name: Code Reviewer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'name' field should be a descriptive name for the workflow, not 'Code Reviewer'.

@@ -0,0 +1,29 @@
name: Code Reviewer
run-name: Action started by ${{ github.actor }}

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 file.

- opened
- synchronize

permissions: write-all

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 a valid field in a workflow file.

openai_key: ${{ secrets.OPENAI_KEY }}
max_tokens: 900
exclude: "**/*.json, **/*.md, **/*.g.dart"
append_prompt: |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'append_prompt' field should not have a '|' character at the end.

for (const file of parsedDiff) {
if (file.to === "/dev/null") continue; // Ignore deleted files
for (const chunk of file.chunks) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable 'messages' is not used in the loop. Consider removing it or using it in the loop.


const prDetails = await getPRDetails();
let diff; // string | null
const eventData = JSON.parse(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable 'diff' is declared but never used. Consider removing it.

@@ -29,6 +29,18 @@ class SimpleChatGptService {
throw error;
}
}

// chatCompletionParams: ChatCompletionParams

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 the 'fromParams' method.

// chatCompletionParams: ChatCompletionParams
async fromParams({ chatCompletionParams }) {
try {
const response = await this.service.chatCompletions(chatCompletionParams);

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 the case when 'response.choices' is empty.

const message = response.choices[0].message.content;
return message;
} catch (error) {
console.error('[SimpleChatGptService]', error);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid logging the error twice. Remove the console.error statement.

console.error('[SimpleChatGptService]', error);
throw error;
}
}
}

module.exports = SimpleChatGptService;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the '\ No newline at end of file' line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant