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] Review according to own guide #2

Merged
merged 1 commit into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
176 changes: 176 additions & 0 deletions GUIDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
## コンポーネントファイル構造

| Meta | Content |
| --------- | ----------------------------------------------- |
| Level | 必須 |
| Link | https://angular.io/guide/styleguide#style-04-06 |
| AI Review | OFF |

### 説明

各コンポーネントは独自のディレクトリを持ち、そのディレクトリ内にはコンポーネントのクラスファイル、テンプレート、スタイル、テストが含まれるべきです。

### 理由

コンポーネントの関連ファイルを一箇所に集めることで、コンポーネントの再利用性を高め、管理を容易にします。また、新しい開発者がプロジェクトに参加した際の学習コストを削減します。

###

```
// 良いコードの例
/src
/app
/button
button.component.ts
button.component.html
button.component.css
button.component.spec.ts
// 悪いコードの例
/src
/components
button.ts
/styles
button.css
/views
button.html
```

### 例外ケース

- ユーティリティ関数やサービスなど、特定のコンポーネントに紐付かない共有ファイルは共通のディレクトリに配置する。

### リファクタリングガイドライン

既存プロジェクトでは、大幅なディレクトリ構造の変更を避け、新規コンポーネントからこの規則に従うこと。時間が許せば、徐々に既存のコンポーネントも新しいファイル構造に移行させる。

---

## 非同期操作の取り扱い

| Meta | Content |
| --------- | ------------------------------------------------------------------------------------------- |
| Level | 必須 |
| Link | https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function |
| AI Review | ON |

### 説明

非同期操作は`async/await`パターンを使用して実装すること。コールバックや`.then()``.catch()`メソッドを用いた Promise チェーンは避ける。

### 理由

`async/await`は非同期コードをより読みやすく、同期的な流れで記述できるためです。コードの可読性とメンテナンス性が向上します。

###

```typescript
// 良いコードの例
async function getUser(id: string): Promise<User> {
try {
const response = await fetch(`/api/users/${id}`);
const user = await response.json();
return user;
} catch (error) {
throw new Error(error);
}
}

// 悪いコードの例
function getUser(id: string): Promise<User> {
return fetch(`/api/users/${id}`)
.then((response) => response.json())
.then((user) => user)
.catch((error) => {
throw new Error(error);
});
Comment on lines +80 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

非同期操作の取り扱いに関するコーディングガイドに従っていません。Promiseチェーンを使用していますが、async/awaitパターンを使用するように改善してください。

Comment on lines +80 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

非同期操作の取り扱いに関するコーディングガイドに従っていません。非同期操作はasync/awaitパターンを使用して実装する必要がありますが、80行目から86行目ではPromiseチェーンを使用しています。async/awaitパターンに書き換える必要があります。

Reference: ## 非同期操作の取り扱い

}
```

### 例外ケース

- 既存のコードベースで`Promise`チェーンが広範に使用されている場合は、段階的に`async/await`にリファクタリングする。

### リファクタリングガイドライン

新規プロジェクトや新規機能では`async/await`を用いること。既存のコードは、重要なバグ修正や機能追加の際に、この新しいパターンに徐々に置き換えていく。

## 型定義の適用

| Meta | Content |
| --------- | ------------------------------------------------------------------ |
| Level | 必須 |
| Link | https://www.typescriptlang.org/docs/handbook/2/everyday-types.html |
| AI Review | ON |

### 説明

全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。

### 理由

型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。

###

```typescript
// 良いコードの例
function add(x: number, y: number): number {
return x + y;
}

// 悪いコードの例
function add(x, y) {
return x + y;
}
Comment on lines +123 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

関数addの引数xとyに明示的な型を指定してください。

Comment on lines +123 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

関数addの引数xとyに明示的な型を指定してください。

Reference: ## 型定義の適用

```

### 例外ケース

- ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。

### リファクタリングガイドライン

既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。

---

## ユニットテストの実施

| Meta | Content |
| --------- | -------------------------------------- |
| Level | 推奨 |
| Link | https://jestjs.io/docs/getting-started |
| AI Review | OFF |

### 説明

新しく追加する全ての関数に対してユニットテストを書き、既存の関数についても段階的にテストを追加する。

### 理由

ユニットテストにより、コードの品質を保ち、将来的な機能追加やリファクタリングが安全に行われるようにします。バグの発見を早期に行い、修正コストを削減する効果もあります。

###

```typescript
// 良いコードの例(テストの例)
describe("add function", () => {
it("adds two numbers correctly", () => {
expect(add(1, 2)).toBe(3);
});
});

// 悪いコードの例(テストがない)
function add(x: number, y: number): number {
return x + y;
}
```

### 例外ケース

- 外部 API の結果に依存する関数など、テストが困難なケースでは、モックを使用して依存関係を切り離す。

### リファクタリングガイドライン

テストがない既存の関数に対しては、新しいバグ報告や機能追加があるたびにユニットテストを追加していく。新規関数では最初からテストを書くことをルール化する。
113 changes: 89 additions & 24 deletions packages/ai-craftsman/src/ci.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,27 @@
import { readFileSync } from "node:fs";
import { parseMarkdown, type MarkdownSectionNode } from "./markdown.js";
import { getDiff, postComment } from "./git.js";
import { splitForEachDIff } from "./diff.js";
import { chat } from "./openai.js";
Comment on lines 4 to 5
Copy link
Contributor

Choose a reason for hiding this comment

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

import文の型指定がありません。型指定を追加してください。

Comment on lines 4 to 5
Copy link
Contributor

Choose a reason for hiding this comment

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

import文の型指定がありません。型指定を追加してください。

Reference: ## 型定義の適用

Comment on lines 4 to 5
Copy link
Contributor

Choose a reason for hiding this comment

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

import文の順番が間違っています。node:fsモジュールは先にインポートする必要があります。

Reference: ## 非同期操作の取り扱い

Comment on lines 4 to 5
Copy link
Contributor

Choose a reason for hiding this comment

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

import文の型指定がありません。型指定を追加してください。

Reference: ## 型定義の適用

import { promiseAllWithConcurrencyLimit } from "./concurrent.js";

Comment on lines 5 to 7
Copy link
Contributor

Choose a reason for hiding this comment

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

非同期操作の取り扱いのコーディングガイドに従っていません。async/awaitパターンを使用して非同期操作を実装する必要があります。

Reference: ## 非同期操作の取り扱い

Comment on lines 5 to 7
Copy link
Contributor

Choose a reason for hiding this comment

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

コーディングガイドに従っていません。コーディングガイドでは非同期操作はasync/awaitパターンを使用するように指示されていますが、このコードではimport文が使用されています。import文は非同期操作ではないため、async/awaitパターンを使用する必要があります。

Reference: ## 非同期操作の取り扱い

const prompt = `\
あなたは世界最高峰のプログラマーです。あなたの仕事はGitHubのdiffを見てコードレビューをすることです。
以下のdiffをレビューして改善点を挙げてください。改善点が特にない場合は 'NO' と回答してください。
以下のdiffに対して以下のコーディングガイドに従っているかをレビューしてください。
コーディングガイドに従っている場合は "OK" とコメントしてください。
コーディングガイドに従っていない場合は改善方法をコメントしてください。
diffの各行の先頭には行番号がついていることに注意してください。
回答形式は以下です。

Comment on lines +10 to 14
Copy link
Contributor

Choose a reason for hiding this comment

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

コーディングガイドに従っていません。コーディングガイドに従っているかどうかを判断するためのコメントが必要です。

Reference: ## 非同期操作の取り扱い

---

コーディングガイドは以下です。

{{CODING_GUIDE}}

---

回答形式は以下です。
Comment on lines +19 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

コーディングガイドが記載されていません。コーディングガイドを記載する必要があります。

Reference: ## 非同期操作の取り扱い


## 改善点
lines: {{指摘開始行}},{{指摘終了行}}
{{ここに改善点を書いてください}}
Expand All @@ -19,13 +31,43 @@ lines: {{指摘開始行}},{{指摘終了行}}
{{ここに改善点を書いてください}}
`;

const buildPrompt = (rule: string) => {
return prompt.replace("{{CODING_GUIDE}}", rule);
};
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数buildPromptの引数ruleに型を指定しましょう。

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数の引数と戻り値に明示的な型を指定しましょう。

Reference: ## 型定義の適用

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数の引数と戻り値に明示的な型を指定しましょう。

Reference: ## 型定義の適用

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

buildPrompt関数の中でprompt.replaceを使用しているが、promptの中には{{CODING_GUIDE}}が含まれていないため、置換が行われない可能性がある。promptの中に{{CODING_GUIDE}}が含まれていることを確認する必要があります。

Reference: ## 非同期操作の取り扱い

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数buildPromptの引数ruleに型が指定されていません。ruleは文字列として扱われているので、型をstringとして明示してください。

Reference: ## 型定義の適用

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数 buildPrompt の引数 rule に型が指定されていません。rule は文字列として扱われているので、型を string として明示的に指定してください。

Reference: ## 型定義の適用

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数buildPromptの引数ruleに型が指定されていません。ruleは文字列として扱われているので、型をstringと明示してください。

Reference: ## 型定義の適用 ,,,| Meta | Content |,| --------- | ------------------------------------------------------------------ |,| Level | 必須 |,| Link | https://www.typescriptlang.org/docs/handbook/2/everyday-types.html |,| AI Review | ON |,### 説明,,,,全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。,### 理由,,,,型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。,### 例,,,,```typescript,// 良いコードの例,function add(x: number, y: number): number {, return x + y;,},,// 悪いコードの例,function add(x, y) {, return x + y;,},```,### 例外ケース,,,,- ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。,### リファクタリングガイドライン,,,,既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。,,---,

Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

関数buildPromptの引数ruleに型が指定されていません。ruleは文字列として扱われているので、型をstringと明示してください。

Reference: ## 型定義の適用
Meta Content
Level 必須
Link https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
AI Review ON

説明

全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。

理由

型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。

// 良いコードの例
function add(x: number, y: number): number {
  return x + y;
}

// 悪いコードの例
function add(x, y) {
  return x + y;
}

例外ケース

  • ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。

リファクタリングガイドライン

既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。



const excludePatterns = [
/.*node_modules/,
/.*pnpm-lock.yaml$/,
/.*yarn.lock$/,
/.*package-lock.json$/,
/.*\.md$/,
];

const readRules = () => {
const getRule = (node: MarkdownSectionNode) => {
let rule = `${node.title}\n\n${node.content}`;
if (node.children.length > 0) {
for (const child of node.children) {
rule += getRule(child);
}
}
return rule;
};
Comment on lines +47 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

関数 getRule の引数 node と戻り値に型が指定されていません。node の型と getRule の戻り値の型を明示的に指定してください。

Reference: ## 型定義の適用


const markdown = readFileSync("GUIDE.md", "utf-8");
const parsed = parseMarkdown(markdown);
const rules: string[] = [];
for (const level1 of parsed.children) {
for (const level2 of level1.children) {
const rule = getRule(level2);
if (rule.match(/AI Review.*ON/g)) {
rules.push(getRule(level2));
}
}
}
return rules;
};
Comment on lines +46 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

関数readRulesに明示的な型を指定しましょう。

Comment on lines +46 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

関数readRulesに明示的な型を指定しましょう。

Reference: ## 型定義の適用

Comment on lines +46 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

関数readRulesの戻り値に型が指定されていません。この関数は文字列の配列を返しているので、戻り値の型をstring[]として明示してください。

Reference: ## 型定義の適用

Comment on lines +47 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

この部分のコードでは、ファイルの読み込みやMarkdownのパースなど非同期操作が含まれていますが、これらの操作はasync/awaitパターンを使用して実装するべきです。現在のコードでは同期的なファイル読み込み関数readFileSyncが使用されていますが、これを非同期のreadFileに変更し、関数readRulesを非同期関数に変更することを推奨します。

改善後のコードは以下のようになります。

const readRules = async () => {
  const getRule = (node: MarkdownSectionNode) => {
    let rule = `${node.title}\n\n${node.content}`;
    if (node.children.length > 0) {
      for (const child of node.children) {
        rule += getRule(child);
      }
    }
    return rule;
  };

  const markdown = await fs.promises.readFile("GUIDE.md", "utf-8");
  const parsed = parseMarkdown(markdown);
  const rules: string[] = [];
  for (const level1 of parsed.children) {
    for (const level2 of level1.children) {
      const rule = getRule(level2);
      if (rule.match(/AI Review.*ON/g)) {
        rules.push(getRule(level2));
      }
    }
  }
  return rules;
};

ここではfs.promises.readFileを使用してファイルを非同期に読み込んでいます。また、関数readRulesは非同期関数として定義されているため、この関数を呼び出す際にはawaitを使用する必要があります。

Reference: ## 非同期操作の取り扱い

Comment on lines +46 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

関数 readRules の戻り値に型が指定されていません。この関数は文字列の配列を返しているので、戻り値の型を string[] として明示的に指定してください。

Reference: ## 型定義の適用

Comment on lines +46 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

関数readRulesの戻り値に型が指定されていません。この関数は文字列の配列を返しているので、戻り値の型をstring[]と明示してください。

Reference: ## 型定義の適用 ,,,| Meta | Content |,| --------- | ------------------------------------------------------------------ |,| Level | 必須 |,| Link | https://www.typescriptlang.org/docs/handbook/2/everyday-types.html |,| AI Review | ON |,### 説明,,,,全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。,### 理由,,,,型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。,### 例,,,,```typescript,// 良いコードの例,function add(x: number, y: number): number {, return x + y;,},,// 悪いコードの例,function add(x, y) {, return x + y;,},```,### 例外ケース,,,,- ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。,### リファクタリングガイドライン,,,,既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。,,---,

Comment on lines +47 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

この部分のコードでは、ファイルを読み込むためにreadFileSyncを使用していますが、これは同期的な操作です。非同期操作の取り扱いに関するコーディングガイドに従うためには、readFileを使用し、その結果をawaitで待つように変更するべきです。また、そのためにはreadRules関数を非同期関数(async function)にする必要があります。

以下に改善後のコードを示します。

const readRules = async () => {
  const getRule = (node: MarkdownSectionNode) => {
    let rule = `${node.title}\n\n${node.content}`;
    if (node.children.length > 0) {
      for (const child of node.children) {
        rule += getRule(child);
      }
    }
    return rule;
  };

  const markdown = await fs.promises.readFile("GUIDE.md", "utf-8");
  const parsed = parseMarkdown(markdown);
  const rules: string[] = [];
  for (const level1 of parsed.children) {
    for (const level2 of level1.children) {
      const rule = getRule(level2);
      if (rule.match(/AI Review.*ON/g)) {
        rules.push(getRule(level2));
      }
    }
  }
  return rules;
};

この変更により、ファイルの読み込みが非同期になり、他の操作がブロックされることなく実行できるようになります。

Reference: ## 非同期操作の取り扱い
Meta Content
Level 必須
Link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function
AI Review ON

説明

非同期操作はasync/awaitパターンを使用して実装すること。コールバックや.then().catch()メソッドを用いた Promise チェーンは避ける。

理由

async/awaitは非同期コードをより読みやすく、同期的な流れで記述できるためです。コードの可読性とメンテナンス性が向上します。

// 良いコードの例
async function getUser(id: string): Promise<User> {
  try {
    const response = await fetch(`/api/users/${id}`);
    const user = await response.json();
    return user;
  } catch (error) {
    throw new Error(error);
  }
}

// 悪いコードの例
function getUser(id: string): Promise<User> {
  return fetch(`/api/users/${id}`)
    .then((response) => response.json())
    .then((user) => user)
    .catch((error) => {
      throw new Error(error);
    });
}

例外ケース

  • 既存のコードベースでPromiseチェーンが広範に使用されている場合は、段階的にasync/awaitにリファクタリングする。

リファクタリングガイドライン

新規プロジェクトや新規機能ではasync/awaitを用いること。既存のコードは、重要なバグ修正や機能追加の際に、この新しいパターンに徐々に置き換えていく。

Comment on lines +46 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

関数readRulesの戻り値に型が指定されていません。この関数は文字列の配列を返しているので、戻り値の型をstring[]と明示してください。

Reference: ## 型定義の適用
Meta Content
Level 必須
Link https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
AI Review ON

説明

全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。

理由

型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。

// 良いコードの例
function add(x: number, y: number): number {
  return x + y;
}

// 悪いコードの例
function add(x, y) {
  return x + y;
}

例外ケース

  • ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。

リファクタリングガイドライン

既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。



const splitComments = (
response: string
): { start: number; end: number; comment: string }[] => {
Comment on lines 71 to 73
Copy link
Contributor

Choose a reason for hiding this comment

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

関数 splitComments の引数 response と戻り値に型が指定されていません。response は文字列として扱われているので、型を string として明示的に指定してください。また、この関数はオブジェクトの配列を返しているので、戻り値の型を { start: number; end: number; comment: string }[] として明示的に指定してください。

Reference: ## 型定義の適用

Comment on lines 71 to 73
Copy link
Contributor

Choose a reason for hiding this comment

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

関数splitCommentsの引数responseと戻り値に型が指定されていません。responseは文字列として扱われているので、型をstringと明示してください。また、この関数はオブジェクトの配列を返しているので、戻り値の型を{ start: number; end: number; comment: string }[]と明示してください。

Reference: ## 型定義の適用 ,,,| Meta | Content |,| --------- | ------------------------------------------------------------------ |,| Level | 必須 |,| Link | https://www.typescriptlang.org/docs/handbook/2/everyday-types.html |,| AI Review | ON |,### 説明,,,,全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。,### 理由,,,,型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。,### 例,,,,```typescript,// 良いコードの例,function add(x: number, y: number): number {, return x + y;,},,// 悪いコードの例,function add(x, y) {, return x + y;,},```,### 例外ケース,,,,- ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。,### リファクタリングガイドライン,,,,既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。,,---,

Comment on lines 71 to 73
Copy link
Contributor

Choose a reason for hiding this comment

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

関数splitCommentsの引数responseと戻り値に型が指定されていません。responseは文字列として扱われているので、型をstringと明示してください。また、この関数はオブジェクトの配列を返しているので、戻り値の型を{ start: number; end: number; comment: string }[]と明示してください。

Reference: ## 型定義の適用
Meta Content
Level 必須
Link https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
AI Review ON

説明

全ての変数、関数の引数、および戻り値には明示的な型またはインターフェースを指定すること。

理由

型を明示的に宣言することで、コンパイル時の型チェックの恩恵を受けられ、ランタイムエラーのリスクを減らします。また、コードの意図が明確になり、他の開発者がコードを理解しやすくなります。

// 良いコードの例
function add(x: number, y: number): number {
  return x + y;
}

// 悪いコードの例
function add(x, y) {
  return x + y;
}

例外ケース

  • ライブラリが提供する関数や変数の型が any である場合は、適宜型アサーションを使用する。

リファクタリングガイドライン

既存のコードでは、任意の型が使われていた場合、それを具体的な型に置き換えていく。新規コードでは初めから型を適用する。


Expand All @@ -52,9 +94,9 @@ const splitComments = (
const comments: { start: number; end: number; comment: string }[] = [];
let buf = "";
const lines =
response.match(/.*?(##\s*改善点[\s\S]*)/m)?.[0].split("\n") ?? [];
response.match(/.*?((#+)?\s*改善点[\s\S]*)/m)?.[0].split("\n") ?? [];
for (const line of lines) {
Comment on lines +97 to 98
Copy link
Contributor

Choose a reason for hiding this comment

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

ここでは非同期操作が行われていますが、async/awaitパターンが使用されていません。コーディングガイドに従って、非同期操作はasync/awaitパターンを使用して実装するように改善してください。

例えば、以下のように修正できます。

async function splitComments() {
  try {
    const response = await fetchResponse();
    const lines = response.match(/.*?((#+)?\s*改善点[\s\S]*)/m)?.[0].split("\n") ?? [];
    // 以下、同様の処理を続ける
  } catch (error) {
    throw new Error(error);
  }
}

ここで、fetchResponse()は非同期でレスポンスを取得する関数とします。

Reference: ## 非同期操作の取り扱い ,,,| Meta | Content |,| --------- | ------------------------------------------------------------------------------------------- |,| Level | 必須 |,| Link | https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function |,| AI Review | ON |,### 説明,,,,非同期操作は`async/await`パターンを使用して実装すること。コールバックや`.then()`、`.catch()`メソッドを用いた Promise チェーンは避ける。,### 理由,,,,`async/await`は非同期コードをより読みやすく、同期的な流れで記述できるためです。コードの可読性とメンテナンス性が向上します。,### 例,,,,```typescript,// 良いコードの例,async function getUser(id: string): Promise {, try {, const response = await fetch(`/api/users/${id}`);, const user = await response.json();, return user;, } catch (error) {, throw new Error(error);, },},,// 悪いコードの例,function getUser(id: string): Promise {, return fetch(`/api/users/${id}`), .then((response) => response.json()), .then((user) => user), .catch((error) => {, throw new Error(error);, });,},```,### 例外ケース,,,,- 既存のコードベースで`Promise`チェーンが広範に使用されている場合は、段階的に`async/await`にリファクタリングする。,### リファクタリングガイドライン,,,,新規プロジェクトや新規機能では`async/await`を用いること。既存のコードは、重要なバグ修正や機能追加の際に、この新しいパターンに徐々に置き換えていく。,

if (line?.match(/^##\s*改善点/)) {
if (line?.match(/^(#+)?\s*改善点/)) {
const comment = build(buf);
if (comment != null) {
comments.push(comment);
Expand All @@ -66,15 +108,34 @@ const splitComments = (
}
}

if (buf !== "") {
const comment = build(buf);
if (comment != null) {
comments.push(comment);
}
}

return comments;
};

const buildComment = (comment: string, rule: string) => {
const [name, ...rest] = rule.split("\n");
const detail = `\
<details>
<summary>Reference: ${name?.replace(/^\s*#+\s*/, "")}</summary>
${rest.join("\n")}
</details>`;

return `${comment}\n\n${detail}`;
};

const main = async () => {
const baseref = process.env["BASE_REF"];
if (baseref == null || baseref === "") {
throw new Error("BASE_REF is not set");
}

const rules = readRules();
const promises: (() => Promise<void>)[] = [];
for (const { diff, path } of getDiff(baseref)) {
if (excludePatterns.some((pattern) => pattern.test(path))) {
Expand All @@ -83,31 +144,35 @@ const main = async () => {
} else {
console.log(`RUN REVIEW: ${path}`);
}
console.log(diff);
const chunked = splitForEachDIff(diff);
for (const source of chunked) {
promises.push(async () => {
const response = await chat([
{
content: prompt,
role: "system",
},
{
content: source.diff,
role: "user",
},
]);

console.log({ response });
for (const comment of splitComments(response)) {
console.log({ comment });
postComment(path, comment.start, comment.end, comment.comment);
}
});
for (const rule of rules) {
promises.push(async () => {
const response = await chat([
{
content: buildPrompt(rule),
role: "system",
},
{
content: source.diff,
role: "user",
},
Comment on lines +149 to +159
Copy link
Contributor

Choose a reason for hiding this comment

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

関数の引数と戻り値に明示的な型を指定しましょう。

Comment on lines +149 to +159
Copy link
Contributor

Choose a reason for hiding this comment

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

関数内の非同期処理をPromise.allで実行するように修正しましょう。

Reference: ## 型定義の適用

]);

for (const comment of splitComments(response)) {
postComment(
path,
comment.start,
comment.end,
`${buildComment(comment.comment, rule)}`
);
}
});
}
}
}

await promiseAllWithConcurrencyLimit(promises, 10);
await promiseAllWithConcurrencyLimit(promises, 1);
};

void main();
23 changes: 15 additions & 8 deletions packages/ai-craftsman/src/openai.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,21 @@ export const chat = async (messages: OpenAI.ChatCompletionMessageParam[]) => {
return acc + getTokenCount(message.content ?? "");
}, 0);

const model =
tokenCount * 1.1 + 1024 < 4 * 1024 ? "gpt-3.5-turbo" : "gpt-3.5-turbo-16k";
const response = await openai.chat.completions.create({
model,
messages,
temperature: 0,
max_tokens: Math.min(1024, (16 * 1024 - tokenCount) * 0.9),
});
const model = tokenCount * 1.1 + 1024 < 8 * 1024 ? "gpt-4" : "gpt-4-32k";
const response = await openai.chat.completions.create(
{
model,
messages,
temperature: 0,
max_tokens: Math.trunc(Math.min(1024, (16 * 1024 - tokenCount) * 0.9)),
},
{
maxRetries: 2,
timeout: Math.trunc(
Math.max(90 * 1000, Math.min(1000 * 20, tokenCount / 10))
),
}
);

return response.choices[0]?.message?.content ?? "";
};