読者です 読者をやめる 読者になる 読者になる

英語苦手だけどプルリクエスト送ってみた話

1ヶ月ちょっと前の話です。
プルリクエストをしたのは人生で2回目で、プルリクエスト初心者ですが、バグと思われる挙動を見つけたので、原因を調査し、プルリクエストに挑戦してみました。

バグと思われる挙動に出会ったきっかけ

RubyのPryという対話型コンソールのgemがありまして、このgemを拡張して何か作れないかなと思い、少しいじっていました。
少しいじっていて分かったのですが、Pryは自分でコマンドを定義して拡張できるようです。

元から存在するコマンドも定義すれば上書きして、自分仕様のコマンドにできるようです。
"ls"というコマンドが元からあるのですが、これを上書きしてちょっと改造していた時のことです。
"ls"コマンドはもともと"Context"というグループに属しているのですが、これを書き換えて"Test"というグループに属する別の動作をするコマンドにしました。
すると、動作自体は変わったのですが、グループは"Context"のままでした。

…あれ?

おかしいなと思い、グループ定義をしている部分のソースを追っかけてみました。

以下が該当のコードです。必要な部分だけ抜き出して、コメントは僕が入れました。
https://github.com/pry/pry/blob/16c08857a69b772775c31f325b482637fdf9d48e/lib/pry/command.rb#L197-L214

def group(name=nil)
  @group ||= if name
    # 引数に名前が渡されていたら、それを使う
    name
  else
    # デフォルトの名前を設定する処理
    # (省略)
  end
end

コマンドを初期化する時にgroupというメソッドを呼ぶとそのコマンドのグループが設定されるようになっています。
この実装だと、まだ定義されていないコマンドのグループを定義する場合はよいのですが、

@group ||= if name

の部分を見て頂ければわかるように、すでに@groupに何か値が格納されていた場合は、グループ名を初期化する処理が行われません。
これでは、既に存在するコマンドのグループ名は上書きできません。
以下のように修正すればちゃんと動きそうです。

def group(name=nil)
  @group = if name
    # 引数に名前が渡されていたら、それを使う
    name
  elsif @group
    @group
  else
    # デフォルトの名前を設定する処理
    # (省略)
  end
end

これはバグなんじゃないか?この程度なら自分でも修正できそう!と思い、プルリクエストしてみることにしました。

プルリクエストしてみる

さて、プルリクするぞ!と意気込んだはいいものの、Githubリポジトリを見る限り、会話はすべて英語なわけで…
「え、英語…(´・ω・`)」と思いつつも、よしおかさん(id:hyoshiok)が英語やできるようになっとかなきゃと何度も言ってるのを思い出し、ググりながらこんな感じで書いてみました。
https://github.com/pry/pry/pull/1066
f:id:kuro_m88:20140201205524p:plain
すると、しばらくして、お返事がきました。
f:id:kuro_m88:20140201210537p:plain
「どんな場面でバグるの?」という事でしたので、
バグる場合のサンプルコードを書いて、簡単に説明しました。
(あと、自信がなかったので、英語下手でごめんなさいとも書いておきました)
翌日、「確かにバグっぽいね」とのコメントがありました。
f:id:kuro_m88:20140201210738p:plain
これは自分のプルリクエストがマージされるチャンスが来たんじゃないか!…と思ったその時
f:id:kuro_m88:20140201211050p:plain
テストコード中に、「グループ名は初期化したら変更されない」というものがあったから、これは正しい動作だよね。という趣旨の指摘を頂きました。
確かに、テストコードを実行してみると、この条件に引っかかってテストがコケていました。
変更を適用してテストが通るか確認する事をすっかり忘れていました…m(_ _)m

結果

テストに通らないどころか、仕様も確認していなかったので、当然マージされることなくクローズされました。テストは大事です…。
これに関連してですが、groupメソッドの説明にグループ名は変更できないという説明が一文書き加えられはしたようです。
https://github.com/pry/pry/pull/1067/files#diff-f252d490dda04825fb67ea3f0b8a558bR197

ここで、グループ名も上書きできるべきだ!という説得力のある理由があれば通ったのかもしれませんが、そこまでの理由もないと思うので、終了になりました。

2回目のプルリクエストをやってみて「GitHubってこんなにも簡単に変更を提案できて、他の人からコメントを貰えるのか!」と改めて思いました。凄いですね。オープンソース素晴らしい。
初めてプルリクエストをした時は、特に会話もなくマージされてしまったので、今回は会話っぽいこともできてよかったです。

また機会があればプルリクエストしてみたいと思います。