-
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
Test PR #1
Test PR #1
Changes from 71 commits
5e39d6a
ea756f4
d1b19df
359de21
38d26f0
1fcadcd
44903e2
d040757
19ece9f
00fbc88
cd1f545
cd7612e
2eea851
d1548d2
c255374
2670f16
3ba739b
9e7b01e
ad28ce7
90edab3
9779458
3392d42
c44418b
f9d9fb8
9532026
13af433
5a1f0b1
9235dd5
84608b5
926f206
08084a8
a17cbdf
1d3de99
f31493d
27ffb14
328e9f7
9c15f54
c892320
6867bab
288b79a
3d3f2da
443ee90
f699e0f
ecfa3df
eda91ff
93b24a1
716ada6
7e0ebe9
c244bfc
e91ae52
6691a7c
72eb518
3f4b20e
2f08e55
434df9b
76d58df
8bf7caf
df108ec
42b7769
78060e8
e1f0363
7fe820f
6660840
312f03f
61746e2
a52477e
764c820
6e79ab1
f54bd21
b73aea8
4795359
89d1351
1c77949
1eb203c
a4d7d5b
d051108
34a9de8
56a1938
042c527
dd04a67
99a9db5
0e6e6b7
4c62433
f1b4568
f1cb5f0
982a3b6
b25ee58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,19 @@ jobs: | |
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v2 | ||
- name: Set up Node | ||
uses: actions/setup-node@v2 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: 18 | ||
- name: Install dependencies | ||
run: npm install | ||
- name: Build Code | ||
run: npm run build | ||
- name: Run Custom Action | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems you've updated the |
||
uses: ./ | ||
with: | ||
github-token: ${{ secrets.GITHUB_TOKEN }} | ||
github_token: ${{ secrets.GITHUB_TOKEN }} | ||
pr-number: ${{ github.event.number }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
openai_api_key: ${{ secrets.OPENAI_API_KEY }} | ||
openai_api_key: ${{ secrets.OPEN_AI_KEY }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The secret key has been changed from |
||
openai_model: 'gpt-4' | ||
review_code: true | ||
expluded_files: 'node_modules, package.json, package-lock.json' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,13 @@ name: 'Github PR Magic' | |
description: 'Automatically reviewing and approving PRs' | ||
author: 'Darrell Richards' | ||
inputs: | ||
github-token: | ||
github_token: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The input parameter change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the input parameter from |
||
description: 'Github token' | ||
required: true | ||
pr-number: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to the previous comment, changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the change on line 5, altering |
||
description: 'PR number' | ||
required: true | ||
excluded_files: | ||
description: 'Files to exclude from review' | ||
required: false | ||
default: 'node_modules, package-lock.json, yarn.lock' | ||
openai_api_key: | ||
description: 'OpenAI API key' | ||
required: true | ||
|
@@ -19,6 +20,14 @@ inputs: | |
description: 'Review code' | ||
required: false | ||
default: true | ||
generate_summary: | ||
description: 'Generates Pull Request summary based on git diff and code changes' | ||
required: false | ||
default: false | ||
overall_code_review: | ||
description: 'Overall code review as a comment' | ||
required: false | ||
default: false | ||
runs: | ||
using: 'node20' | ||
main: 'lib/index.js' |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,40 +1,203 @@ | ||
const parser = require('@tandil/diffparse'); | ||
import { readFileSync } from "fs" | ||
import OpenAI from "openai" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
import core from "@actions/core"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove commented out code if it's not intended to be used. It can cause confusion and make the codebase harder to maintain. |
||
import { PRDetails } from "./lib/github"; | ||
import { minimatch } from "minimatch"; | ||
import parseDiff, { File } from "parse-diff" | ||
import * as core from "@actions/core" | ||
|
||
import { compareCommits, createReviewComment, gitDiff, PRDetails, updateBody } from "./services/github"; | ||
import { filter, minimatch } from "minimatch"; | ||
import { obtainFeedback, prSummaryCreation, summaryAllMessages, summaryOfAllFeedback, validateCodeViaAI } from "./services/ai"; | ||
|
||
|
||
const excludedFiles = core.getInput("excluded_files").split(",").map((s: string) => s.trim()); | ||
const createPullRequestSummary = core.getInput("generate_summary"); | ||
const reviewCode = core.getInput("review_code") | ||
const overallReview = core.getInput("overall_code_review"); | ||
|
||
|
||
export interface Details { | ||
title: string; | ||
description: string; | ||
} | ||
|
||
async function validatePullRequest(diff: File[], details: Details) { | ||
const foundSummary = []; | ||
for (const file of diff) { | ||
for (const chunk of file.chunks) { | ||
const message = await prSummaryCreation(file, chunk, details); | ||
if (message) { | ||
const mappedResults = message.flatMap((result: any) => { | ||
if (!result.changes) { | ||
return []; | ||
} | ||
|
||
if (!result.typeChanges) { | ||
return []; | ||
} | ||
|
||
if (!result.checklist) { | ||
return []; | ||
} | ||
|
||
|
||
return { | ||
changes: result.changes, | ||
typeChanges: result.typeChanges, | ||
checklist: result.checklist, | ||
}; | ||
}); | ||
|
||
foundSummary.push(...mappedResults); | ||
} | ||
} | ||
} | ||
|
||
|
||
if (foundSummary && foundSummary.length > 0) { | ||
const bodyIdea = await summaryAllMessages(foundSummary); | ||
return bodyIdea; | ||
} | ||
|
||
return ''; | ||
} | ||
|
||
async function validateCode(diff: File[], details: Details) { | ||
const neededComments = []; | ||
for (const file of diff) { | ||
for (const chunk of file.chunks) { | ||
const results = await validateCodeViaAI(file, chunk, details); | ||
|
||
if (results) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition |
||
const mappedResults = results.flatMap((result: any) => { | ||
if (!file.to) { | ||
return []; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like |
||
|
||
if (!result.lineNumber) { | ||
return []; | ||
} | ||
|
||
if (!result.review) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing |
||
return []; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
return { | ||
body: result.review, | ||
path: file.to, | ||
position: Number(result.lineNumber), | ||
}; | ||
}); | ||
|
||
if (mappedResults) { | ||
neededComments.push(...mappedResults); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the function |
||
} | ||
} | ||
|
||
return neededComments; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable |
||
async function validateOverallCodeReview(diff: File[], details: Details) { | ||
const detailedFeedback = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
for (const file of diff) { | ||
for (const chunk of file.chunks) { | ||
const results = await obtainFeedback(file, chunk, details); | ||
if (results) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the log for generating a summary of new PRs has been removed. This information could be useful for debugging. Consider adding it back or replacing it with a more informative message, especially if other logging mechanisms haven't been provided elsewhere. |
||
const mappedResults = results.flatMap((result: any) => { | ||
if (!file.to) { | ||
return []; | ||
} | ||
|
||
return { | ||
changesOverview: result.changesOverview, | ||
feedback: result.feedback, | ||
improvements: result.improvements, | ||
conclusion: result.conclusion, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable |
||
}); | ||
|
||
if (mappedResults) { | ||
detailedFeedback.push(...mappedResults); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return detailedFeedback; | ||
} | ||
|
||
const openai = new OpenAI() | ||
const excludedFiles = core.getInput("exclude").split(",").map((s: string) => s.trim()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that |
||
|
||
async function main() { | ||
let dif: string | null = null; | ||
const { action, repository, number } = JSON.parse(readFileSync(process.env.GITHUB_EVENT_PATH || "", "utf-8")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary comments and commented out code should be removed before merging to keep the codebase clean. |
||
const { title, description } = await PRDetails(repository, number); | ||
const { action, repository, number, before, after } = JSON.parse(readFileSync(process.env.GITHUB_EVENT_PATH || "", "utf-8")) | ||
const { title, description, patch_url, diff_url } = await PRDetails(repository, number); | ||
|
||
|
||
if (action === "opened") { | ||
// Generate a summary of the PR since it's a new PR | ||
|
||
const data = await gitDiff(repository.owner.login, repository.name, number); | ||
dif = data as unknown as string; | ||
} else if (action === "synchronize") { | ||
const newBaseSha = before; | ||
const newHeadSha = after; | ||
|
||
const data = await compareCommits({ | ||
owner: repository.owner.login, | ||
repo: repository.name, | ||
before: newBaseSha, | ||
after: newHeadSha, | ||
number | ||
}) | ||
|
||
dif = String(data); | ||
} else { | ||
console.log('Unknown action', process.env.GITHUB_EVENT_NAME); | ||
return; | ||
} | ||
|
||
if (!dif) { | ||
// Well shit. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The nullish coalescing operator ( |
||
return; | ||
} | ||
|
||
const diff = parser.parseDiffString(dif); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
const filteredDiff = diff.filter((file: { to: any; }) => { | ||
const diff = parseDiff(dif); | ||
const filteredDiff = diff.filter((file) => { | ||
return !excludedFiles.some((pattern) => | ||
minimatch(file.to ?? "", pattern) | ||
); | ||
}); | ||
|
||
console.log(filteredDiff); | ||
if (reviewCode) { | ||
// @ToDo Improve the support for comments, | ||
// IE: Remove outdated comments when code is changed, revalidated if the Pull Request is ready to be approved. | ||
const neededComments = await validateCode(filteredDiff, { | ||
title, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems there is a missing |
||
description | ||
}); | ||
|
||
await createReviewComment(repository.owner.login, repository.name, number, neededComments); | ||
} | ||
|
||
const detailedFeedback = await validateOverallCodeReview(diff, { | ||
title, | ||
description | ||
}); | ||
|
||
if (detailedFeedback && detailedFeedback.length > 0) { | ||
console.log('feedbacks_index', detailedFeedback); | ||
const resultsFullFeedback = await summaryOfAllFeedback(detailedFeedback); | ||
|
||
// Validate Some Code Yo! | ||
console.log('feedback_result', resultsFullFeedback); | ||
} | ||
|
||
// Post some comments | ||
if (action === "opened" && createPullRequestSummary) { | ||
console.log('Generating summary for new PR'); | ||
const summary = await validatePullRequest(diff, { | ||
title, | ||
description | ||
}); | ||
|
||
await updateBody(repository.owner.login, repository.name, number, summary) | ||
} | ||
} | ||
|
||
main(); |
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.
It is good to see the action updated to newer versions; however, there is a minor typo in the property name
expluded_files
. It should beexcluded_files
.