メリークリスマスイブ！
こんにちは。Supership株式会社アドテクノロジーセンターのjosukeです。
この記事は、Supershipグループ Advent Calendar 2023の24日目の記事になります。
はじめに
エンジニアにとって、コードレビューは日々の仕事の中でも欠かせない部分ですよね。
私たちの多くが、同僚のフィードバックを受けて、自分のコードをより良くしていると思います。
この記事では特に、レビューを受ける側の視点に焦点を当てていきます。
私自身、レビューを受ける際、ケアレスミスが目立つと感じることがあります。
レビューコメントから学ぶことは多いですが、同時に、細かなミスを指摘されるのは避けたいものです。
レビュイーとしては、前のタスクのレビューに対応することで新たに取り組んでいるタスクが中断されるのは避けたいですし、レビュワーには非本質的な部分のチェックを負担として課したくありません。
そこで、レビューリクエストを提出する前に、GitHub Copilot Chatで一度レビューを行うことで、ケアレスミスに気づき、レビューでの指摘回数を減らせないかと考え、試してみました。
TL;DR
Github Copilot Chatにgitのdiffを渡して、ケアレスミスを指摘してもらえるか試しました。
具体的なミスの内容としては、
- 日本語の誤字
- コメントとコード間の矛盾
- デバッグ用コードの消し忘れ
の3種類を検出できるか試しました。
自分が試した範囲では、以下のことがわかりました。
- 小さくてシンプルな変更をコードベースに加えたとき、GitHub Copilot Chatはケアレスミスを検出することができる
- ただし、どのような観点でレビューして欲しいかを具体的に指示する必要がある
- 差分が大きくなると、ミスを検出しにくくなる
前提
GitHub Copilot Chatにgitのdiffを渡してレビューをお願いします。
また、レビュー対象はダミーのRailsプロジェクトです。
レビュー対象となる差分は全てUser関連の処理です。
Userテーブルの構成は下記の図のように、ごく一般的なものです。
レビュー項目
日本語の誤字がないか
日本語で入力されたコメントやテストケース名に誤字がある場合に指摘してもらえるか試してみます。
英語で書く箇所はCode Spell Checkerなどで検出できるので、日本語のみチェックします。
下記のスニペットのように、Userモデルのテストにメールアドレスのバリデーションのテストを追加するケースを考えます。
contextの説明を見ると、「有効なemail」が「有向なemail」になってしまっています。
require 'rails_helper'
RSpec.describe User, type: :model do
let(:user) { FactoryBot.build(:user) }
describe 'name' do
context '名前が空文字の場合' do
before { user.name = '' }
it 'バリデーションに失敗する' do
expect(user).not_to be_valid
end
end
end
#### 変更箇所 ####
describe 'email' do
context '有向なemailの場合' do # 有効が有向になっている
before { user.email = 'test@example.com' }
it do
expect(user).to be_valid
end
end
end
end
では、実際にdiffを渡してレビューをお願いしてみます。
すると、次のような回答が返ってきました。
変更の意図は汲み取れていますが、誤字の指摘はされていないですね...
そこで、プロンプトに「日本語が記述されている箇所について、スペルミスがないかチェックをお願いします。」と追加してみます。
プロンプト修正後の回答は、
でした。
誤字の指摘が正しく行われていますね。
GitHub Copilotは日本語の誤字チェックに使えると言えそうです。
コメントとコードに矛盾がないか
既存のコードを編集するとき、油断しているとコメントとコードの使用に矛盾を生んでしまうことがあります。
このミスは個人的にやりがちなので、指摘してもらえるとありがたいです。
これを指摘してもらえるか試してみます。
先ほど使用したUserモデルのテストに、さらにテストケースを追加してみます。
describe 'password' do
context 'パスワードが6文字以上の場合' do
before { user.password = "a" * 6 }
it do
expect(user).to be_valid
end
end
end
セキュリティ向上のためにパスワードの文字数下限を8文字に変更したとき、テストケースは変更したが、その説明文の修正を忘れた場合を想定してレビューをお願いしてみます。
誤字の指摘をお願いしたときと同様に、これだけだとミスを指摘してもらえませんでした。そこで、「特に、コメントとコードの間で矛盾が生じていないかチェックしてください」という文言を追加してみたところ次の回答が得られました。
こちらも具体的にレビューして欲しい箇所を指定することでミスを検出できました。
消し忘れたデバッグ用コードは無いか
デバッグ用コードの消し忘れを検出できるか試してみます。
binding.pry のようなデバッガを起動するコードはCIで検出できるはずなので、printデバッグが検出できるかをチェックしてみます。
Usersコントローラのcreateアクションを作成する際に、フォームからどのような値が送られているかチェックするためのprintデバッグを消し忘れた想定でコードを書いてみます。
def create
logger.debug "user_params: #{user_params}" # 消し忘れ
user = User.new(user_params)
if user.save
redirect_to user
else
render :new
end
end
def show
例によってレビューして欲しい箇所を指定しつつ、diffを渡します。
すると、次のような回答が返ってきます。
不要なデバッグ用コードの指摘もできていますね。
全部まとめてチェック
実際に業務で使うことを考えると、プルリクエストを出す前にブランチごとに比較することが予想されます。
ある程度大きいdiffを渡して、それに対してミスを検出できないと実用に耐えないと思われます。
今までに扱ったdiffに加え、関係ないコミットを少し混ぜた上でレビューを依頼してみます。
このとき、レビューして欲しい項目をそれぞれ指定しています。
返答は次のようなものでした。
日本語のスペルミスとデバッグ用コードの消し忘れは指摘できていますが、コメントとコード間の矛盾を指摘できなくなってしまいました。
まとめ
Github Copilot Chatを使ってケアレスミスを減らす試みをやってきました。
自分が試した範囲では、以下のことがわかりました。
- 小さくてシンプルな変更をコードベースに加えたとき、GitHub Copilot Chatはケアレスミスを検出することができる
- ただし、どのような観点でレビューして欲しいかを具体的に指示する必要がある
- 差分が大きくなると、ミスを検出しにくくなる
まだ実務ではほとんど使えていないので、実務でどの程度役に立つかや、n+1問題などのロジックに関わる部分のミスも指摘できるかを確かめたいと考えています。
おまけ
前から GitHub Copilot Chatにレビューをしてもらいたいという考えはあったのですが、細かい指示やdiffのコピペが面倒でなかなか試せていませんでした。
今回記事を書くにあたって、次のようなプロンプト生成Rubyスクリプトを作りました。
require 'optparse'
DEFAULT_BRANCH = "main".freeze
def main
opt = OptionParser.new
option = {}
opt.on('-c', '--commit') {|v| option[:commit] = v}
opt.on('-n', '--n_plus_one') {option[:n_plus_one] = true}
opt.on('-s', '--spell') {option[:spell] = true}
opt.on('-n', '--nil_care') {option[:nil_care] = true}
opt.on('-w', '--wrong_comment') {option[:nil_care] = true}
opt.on('-d', '--dead_code') {option[:dead_code] = true}
arg = opt.parse!(ARGV)
script = "以下にコードの差分を示すので、これに対してレビューをお願いします。\n"
script += "n+1問題が発生していないかチェックをお願いします。\n" if option[:n_plus_one]
script += "日本語が記述されている箇所について、スペルミスがないかチェックをお願いします。\n" if option[:spell]
script += "nilが返ってくる可能性がある箇所について、nilガードがされているかチェックをお願いします。\n" if option[:nil_care]
script += "コメントとコードで矛盾している箇所がないかチェックをお願いします。\n" if option[:wrong_comment]
script += "デッドコードや意味のないコメントアウトが残っていないかチェックをお願いします。\n" if option[:dead_code]
script += "```txt\n"
diff = ""
if option[:commit]
diff = diff_between_commits(arg[0])
else
current_branch = `git rev-parse --abbrev-ref HEAD`
diff = diff_between_branches(DEFAULT_BRANCH, current_branch)
end
script += "\n#{diff}"
script += "\n```"
puts script
end
def diff_between_branches(base_branch, target_branch)
`git diff #{base_branch}...#{target_branch}`
end
def diff_between_commits(target_commit)
`git diff #{target_commit}^!`
end
main
実行結果をパイプでpbcopyに渡せば、あとはペーストするだけです。
ShellScriptで書けばpbcopyを書く手間も省けると思ったのですが、コマンドライン引数の処理を書くのが面倒そうだったのでRubyで書きました。
最後に宣伝です。
Supershipではプロダクト開発やサービス開発に関わる人を絶賛募集しております。
ご興味がある方は以下リンクよりご確認ください。
Supership 採用サイト
是非ともよろしくお願いします。