技術的な話は全くないので、Qiitaの中の人に消されなくもなさそうですが、うれしかったので。
初めてのPR。そしてマージ
今回、アドベントカレンダーのネタ探しとして、自作mrbgemのエラーをとる作業 をしていたんですが、その中でmruby本体のマクロのタイポを見つけました。修正前のコードでも何の問題もなく動く(何故動くのかわかりませんが)のでこの修正は何にも寄与しないPRですが、これがmrubyへの初めてのPRになります。PRを送ってからその日のうちにマージされたスピード感が半端ないです。
ただ、単純な誤字の指摘。みたいなものなので、あまり達成感はないというか。そういうワガママな気持ちはありました。
2回目のPR。テスト実行時の表示フォーマットの修正
だれかWIndowsでmrubyのテストを実行して、mruby-socketの通らないテストを直す(またはスキップする)修正を行ってくれる人がいないかなあ。私がAppVeyorを頼りに直すのは効率が悪すぎる
— Yukihiro Matsumoto (@yukihiro_matz) December 8, 2017
matzさんからの嘆きのツイートが目に留まったので、一体何が起こっているのか？ということで、自分の環境(windows)でテストを通してみることにしました。確かにエラーがいくつか表示されていました。それと同時に、Failの時には表示されているmrbgemの名前が、Skipの時には表示されていないことに気付きました。
実装を見てみると、failの時とskipの時でメッセージの作成方法に違いがあるのがわかりました。
def assert(str = 'Assertion failed', iso = '')
t_print(str, (iso != '' ? " [#{iso}]" : ''), ' : ') if $mrbtest_verbose
begin
$mrbtest_assert = []
$mrbtest_assert_idx = 0
yield
if($mrbtest_assert.size > 0)
$asserts.push(assertion_string('Fail: ', str, iso, nil)) # Failの時は assertion_stringを呼んでいる
$ko_test += 1
t_print('F')
else
$ok_test += 1
t_print('.')
end
rescue Exception => e
bt = e.backtrace if $mrbtest_verbose
if e.class.to_s == 'MRubyTestSkip'
$asserts.push "Skip: #{str} #{iso} #{e.cause}" # Skipの時は 自前で文字列を作成している
t_print('?')
else
$asserts.push(assertion_string("#{e.class}: ", str, iso, e, bt))
$kill_test += 1
t_print('X')
end
ensure
$mrbtest_assert = nil
end
t_print("\n") if $mrbtest_verbose
end
それを修正し、以下のようにPRを作成。
Failと同じようにSkipでもassertion_stringを使うようにしました。また、Skipの時に呼ばれる MRubyTestSkipはmessageメソッドがないので、causeメソッドの文字列があれば、そちらを使うように修正してあります。
実は何か深い意図があってそうなってるのかな？思想がわからない状態でPRだすのは大丈夫なのかな？とドキドキしながらPRを出しましたが、無事マージされました。よかった。
続けざまの3回目PR。テストをちゃんとスキップさせる(1つだけ)
上記のmatzさんの嘆きのように、Windows のMSVCを使うと、いくつかのテストはこけてしまいます。できれば全部通るようにしたかったのですが、技術がなかったので、私でも内容のわかる1ケースだけスキップするようにしました。それが、以下のPRになります。
何をしてるかというと、File.readlink() はwindowsではサポートされていない機能になるので、NotImplementedError が返ってくるのですが、修正前のrescueではこのエラーが捕捉しきれずにassert_raiseがRuntimeErrorにマッチせずにFailするという問題が起こっていました。なので、NotImplementedErrorも捕捉するように修正し、その外側のresqueで再度捕捉してskipしてもらうようにしています。
こちらも変数名をもっとしっかり考えて付けたほうがいいんじゃないか、とか、この方法よりもっときれいに記述する方法があるんじゃないか？といった不安があり、マージされないんじゃないか。と思っていましたが、マージされました。ありがたい。
なぜ 捕捉できないのか
rescue節に例外の型を何も指定しないで記述した場合、捕捉されるのはStandardErrorのみとなります。NotImplementedErrorはScriptErrorの継承になるので、明示して捕捉してあげる必要があるようです。これはrubyも同じのようです。
そのあたりは、以下の記事に詳しく乗っています。
rescue節で例外の型を指定しない場合に補足できるのはStandardErrorとそのサブクラスだけ
まとめ
今回の3つのPRを通して感じたことをまとめます。
githubのPR機能便利
リポジトリのファイルを直接編集してPRを作成できるので、ちょい修正ぐらいであればフォークして修正してってやるよりも簡単に作成できる。これがマージする側にどういった影響を与えているのかはよくわからないので、もしかしたら今回はmatzさんに負担を強いているかもしれない。。。
もっと楽な気持ちでPRしてもいいんだ
以前は、こんな稚拙なPR出してもマージされないとか、迷惑をかけるんじゃないか、とか、ちゃんとした英語かかなきゃ。みたいな気持ちが先行していて、PRへのハードルが結構高かったんですが、OSS界隈の人たちは優しい人たちが多いので、もっと楽な気持ちでPR出してもいいのかな？と思いました。とはいえ、相手にとって失礼のないようにはしないといけないのと思うので、自分ができる最大の礼儀でもって対応すればいいと思います。丁寧な姿勢でPRを出せば、丁寧に対応してくれます。これはコミュニケーションの基本ですね。
Windows の対応って大変
今回、テストで失敗するケースで修正できたのは1件だけでしたが、他にも修正できるテストケースはないかと調べたところ、Windows への対応って大変だなぁという感想になりました。mingwやcygwinにも対応させようと思うとだいぶカオスな状態になるのだとおもいます。地道に対応していくしかないんでしょうね。
色々書きましたけど、何が言いたいかというと、みなさんもっとmrubyにPRを投げてより良いものにしていきましょう！ってことです。