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

feat: move git commands out of review lambda #46

Merged
merged 5 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/common/git/getChangedFileLines.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { exec } from "child_process";

import { getGitHubEnvVariables } from "../../config";

export const getChangesFileLinesCommand = (
isCi: boolean,
fileName: string
): string => {
if (isCi) {
const { githubSha, baseSha } = getGitHubEnvVariables();
return `git diff -U0 --diff-filter=AMT ${baseSha} ${githubSha} ${fileName}`;
}
return `git diff -U0 --diff-filter=AMT --cached ${fileName}`;
};

export const getChangedFileLines = async (
isCi: boolean,
fileName: string
): Promise<string> => {
const commandString = getChangesFileLinesCommand(isCi, fileName);

return new Promise((resolve, reject) => {
exec(commandString, (error, stdout, stderr) => {
if (error) {
reject(new Error(`Failed to execute command. Error: ${error.message}`));
} else if (stderr) {
reject(new Error(`Command execution error: ${stderr}`));
} else {
const changedLines = stdout
.split("\n")
.filter((line) => line.startsWith("+") || line.startsWith("-"))
.filter((line) => !line.startsWith("---") && !line.startsWith("+++"))
.join("\n");
resolve(changedLines);
}
});
});
};
34 changes: 34 additions & 0 deletions src/common/git/getChangedFilesNames.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { exec } from "child_process";
import { join } from "path";

import { getGitHubEnvVariables } from "../../config";

export const getChangedFilesNamesCommand = (isCi: boolean): string => {
if (isCi) {
const { githubSha, baseSha } = getGitHubEnvVariables();
return `git diff --name-only --diff-filter=AMT ${baseSha} ${githubSha}`;
}
return "git diff --name-only --diff-filter=AMT --cached";
};

export const getChangedFilesNames = async (
isCi: boolean
): Promise<string[]> => {
const commandString = getChangedFilesNamesCommand(isCi);

return new Promise((resolve, reject) => {
exec(commandString, (error, stdout, stderr) => {
if (error) {
reject(new Error(`Failed to execute command. Error: ${error.message}`));
} else if (stderr) {
reject(new Error(`Command execution error: ${stderr}`));
} else {
const files = stdout
.split("\n")
.filter((fileName) => fileName.trim() !== "")
.map((fileName) => join(process.cwd(), fileName.trim()));
resolve(files);
}
});
});
};
23 changes: 23 additions & 0 deletions src/common/git/getFilesWithChanges.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { readFile } from "fs/promises";
import { getChangedFileLines } from "./getChangedFileLines";
import { getChangedFilesNames } from "./getChangedFilesNames";
import { File } from "../types/File";

export const getFilesWithChanges = async (isCi: boolean): Promise<File[]> => {
try {
const fileNames = await getChangedFilesNames(isCi);

const files = await Promise.all(
fileNames.map(async (fileName) => {
const fileContent = await readFile(fileName, "utf8");
const changedLines = await getChangedFileLines(isCi, fileName);

return { fileName, fileContent, changedLines };
})
);

return files;
} catch (error) {
throw new Error(`Failed to get files with changes: ${error}`);
}
};
5 changes: 5 additions & 0 deletions src/common/types/File.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export interface File {
fileName: string;
fileContent: string;
changedLines: string;
}
1 change: 1 addition & 0 deletions src/common/types/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { File } from "./File";
6 changes: 5 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ const main = async () => {
break;
case "review":
const { review } = await import("./review");
await review(argv);
const { getFilesWithChanges } = await import(
"./common/git/getFilesWithChanges"
);
const files = await getFilesWithChanges(argv.ci);
await review(argv, files);
break;
case "test":
const { test } = await import("./test");
Expand Down
10 changes: 6 additions & 4 deletions src/review/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import { commentPerFile } from "../common/ci/commentPerFile";
import { signOff } from "./constants";
import { askAI } from "./llm/askAI";
import { constructPromptsArray } from "./prompt/constructPrompt/constructPrompt";
import { getFileNames } from "./prompt/filesNames/getFileNames";
import { File } from "../common/types";
import { filterFiles } from "./prompt/filterFiles";

interface ReviewArgs {
[x: string]: unknown;
Expand All @@ -14,16 +15,17 @@ interface ReviewArgs {
$0: string;
}

export const review = async (yargs: ReviewArgs) => {
export const review = async (yargs: ReviewArgs, files: File[]) => {
const isCi = yargs.ci;
const shouldCommentPerFile = yargs.commentPerFile;

const modelName = yargs.model as string;

const filteredFiles = filterFiles(files);

const maxPromptLength = getMaxPromptLength(modelName);

const fileNames = await getFileNames(isCi);
const prompts = await constructPromptsArray(fileNames, maxPromptLength, isCi);
const prompts = await constructPromptsArray(filteredFiles, maxPromptLength);

const { markdownReport: response, feedbacks } = await askAI(
prompts,
Expand Down
18 changes: 11 additions & 7 deletions src/review/prompt/constructPrompt/constructPrompt.test.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,27 @@
import { readFile } from "fs/promises";
import { join } from "path";
import { constructPromptsArray } from "./constructPrompt";

describe("When a file is longer than the max prompt length", () => {
jest.setTimeout(30000);
test("constructPromptsArray splits up the file into prompts", async () => {
const testFilePath = join(__dirname, "../../../testFiles/longFile.tsx");
const fileNames = [testFilePath];
const testFileContent = await readFile(testFilePath, "utf8");
const files = [
{
fileName: testFilePath,
fileContent: testFileContent,
changedLines: testFileContent,
},
];
const maxPromptLength = 2000;
const stringifyMargin = 1.2;
const isCi = false;

const result = await constructPromptsArray(
fileNames,
maxPromptLength,
isCi
);
const result = await constructPromptsArray(files, maxPromptLength);

expect(result).toBeInstanceOf(Array);
expect(result.length).toBeGreaterThan(0);
console.log(result[0]);
Copy link
Owner

Choose a reason for hiding this comment

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

Probs dont need to log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, forgot to remove it, thanks !


for (const prompt of result) {
expect(prompt.length).toBeLessThanOrEqual(
Expand Down
48 changes: 18 additions & 30 deletions src/review/prompt/constructPrompt/constructPrompt.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import { readFile } from "fs/promises";
import { makeSlimmedFile } from "../makeSlimmedFile";
import { instructionPrompt } from "../prompts";
import { File } from "../../../common/types";
import { ReviewFile } from "../types";

const getSizeOfReviewFile = (file: ReviewFile): number =>
file.fileName.length + file.fileContent.length;

const splitFilesIntoBatches = async (
files: ReviewFile[],
maxBatchSize: number,
isCi: boolean
files: File[],
maxBatchSize: number
): Promise<ReviewFile[][]> => {
const batches: ReviewFile[][] = [];
let currentBatch: ReviewFile[] = [];
Expand All @@ -20,14 +19,22 @@ const splitFilesIntoBatches = async (
console.warn(
`File ${file.fileName} is larger than the max prompt length, consider using a model with a larger context window. Attempting to slim the file...`
);
const slimmedFile = await makeSlimmedFile(file, maxBatchSize, isCi);
const slimmedFile = await makeSlimmedFile(file, maxBatchSize);
currentBatch.push(slimmedFile);
} else if (currentBatchSize + currentFileSize > maxBatchSize) {
batches.push(currentBatch);
currentBatch = [file];
currentBatch = [
{
fileName: file.fileName,
fileContent: file.fileContent,
},
];
currentBatchSize = currentFileSize;
} else {
currentBatch.push(file);
currentBatch.push({
fileName: file.fileName,
fileContent: file.fileContent,
});
currentBatchSize += currentFileSize;
}
}
Expand All @@ -40,33 +47,14 @@ const splitFilesIntoBatches = async (
return batches;
};

const readFiles = async (fileNames: string[]): Promise<ReviewFile[]> => {
const files: ReviewFile[] = [];

for (const fileName of fileNames) {
try {
const fileContent = await readFile(fileName, "utf8");
files.push({ fileName, fileContent });
} catch (error) {
console.error(`Failed to process file ${fileName}: ${error}`);
}
}

return files;
};

export const constructPromptsArray = async (
fileNames: string[],
maxPromptLength: number,
isCi: boolean
files: File[],
maxPromptLength: number
): Promise<string[]> => {
const filesToReview = await readFiles(fileNames);

const maxPromptPayloadLength = maxPromptLength - instructionPrompt.length;
const promptPayloads = await splitFilesIntoBatches(
filesToReview,
maxPromptPayloadLength,
isCi
files,
maxPromptPayloadLength
);

const prompts = promptPayloads.map((payload) => {
Expand Down
27 changes: 0 additions & 27 deletions src/review/prompt/fileLines/getChangedLines.ts

This file was deleted.

31 changes: 0 additions & 31 deletions src/review/prompt/filesNames/getFileNames.test.ts

This file was deleted.

33 changes: 0 additions & 33 deletions src/review/prompt/filesNames/getFileNames.ts

This file was deleted.

23 changes: 0 additions & 23 deletions src/review/prompt/filesNames/getFileNamesFromGit.ts

This file was deleted.

Loading