大阪丼にプルリク投げた件 その2 まとまりのない感想文

はじめてのGitHubプルリクがマージされました!

こんにちは。書いてる時間はこんばんはですが。

さて、タイトル通りに初めてGitHubでプルリクを投げました。 それが無事にマージされましたので、ここで纏めておきたいと思います。

どこにPR出したの??

何したの

MastodonのAPIを叩くライブラリがRustでありまして、こないだのOpenSSL 1.1.1で reqwestというネットワークのライブラリが更新されました。 これはOpenSSL 1.1.1に対応できるように更新されたのですが、 コレ結構な変更があったみたいです。

そのままではOpenSSL 1.1.1に対応していないのでエラーに。 手元で依存したreqwestのバージョンを上げた 結果reqwestが依存したhyperというライブラリーが更新され、 そのhyperはヘッダーを扱う部分が大分変更されていてそのままではコンパイルできませんでした。

何が欠けていたかというと、ヘッダーまわりの処理するものがまるっと消えていました。 アレコレ調べてみると、hyperxというライブラリがあって、ヘッダーの処理それだけ がなぜか分離されてあって、うまいことソイツとアレコレすることで 一応動くようになったんですね。

さて、ここで私は意気揚々とPRを出そうとしたのです。

PR出してからマージされるまでの間

PR出したとき

PRの仕方とかいろいろ調べましたよ。

では、ここでもう一度PRのコミット履歴ややりとりを見ていだたきだい。 実に"動いた"コードをPRしていたのである。

はじめてコメントがつく

このPR、9/25に出したものです。

で、マージされたのが11/10。

はじめてコメントがついたのが11/4。

オーナーのレビュー

まずここで反省点。

  1. PRはもうマージしてかまわないと思えるくらいに完成されているべきである。
  2. 変更すべきところ以外は触らない。
  3. コードは限りなくシンプルにしなければならない。
  4. もちろんながら、そして結構重要だけども言語に詳しくないといけない。
    1. でもそこまでガチガチにならなくていいっぽい。
    2. 今回はRustにメチャ詳しい人で、すごい的確にレビューしてくれたのでいいかんじになった。

やっぱりマージする側からすると、読みやすいコードである必要があるんですよね。 自分のつくったモノが汚なくなるのは避けたい。

変更履歴がメチャクチャになるような、変な変更は含みたくない。実にあたりまえである。

マージ直前のコメント

レビューの結果指摘された部分を修正して、ほぼ完成(その後そのままマージされたのでその当時は) したところでこういうコメントが来ました。

ao2

@inux39 maybe you can "rebase -i" and "push --force" the topic branch before it gets merged, to have a cleaner development history. A commit message saying "instead into" can become confusing after some time has passed.

つまり、コミットメッセージが適当すぎて困惑する、rebaseして送り直せというもの。

で、これ誰が言ってきたというと、contributorでもなくownerでもない人からでした。 これどーすればええんや、と思いつつも放置しておくことにしました。 ownerから言われてやればいいやと。

っていうかそもそも、このリポジトリのマージはsquashされてマージされてるっぽくて、 だからこそどーでもいいコミットメッセージを使っていたところもあって、 対応するほどでもねえなっていうことでそうしたんですよね。

と、そこへownerがやってきました。

Aaronepower

@ao2 That isn't necessary as I squash and merge all pull requests made to my repositories so they'll be a single commit.

まあ結果なにもしなくてもいいんでした。

マージ・マージ後

さて、無事マージされたのですが、気になるコメントが。

ao2

@Aaronepower in this case squashing the changes is fine, thank you, but please consider evaluating case by case if squashing is the best course of action. Sometimes it really makes sense to have multiple logically separate commits to address an issue.

Also, when you squash, do not blindly squash the commit messages; for instance the "fix requested changes" and "instead into" messages should not belong to the final commit message, and they can even be confusing to a reader.

Having said that, rest assured that your work on Mammut is appreciated, in any way you decide to do it.

確かにsquashされた結果コミットメッセージにそれらが含まれていました。

個人的にコミットメッセージで重要なのはGitHubなんかでデカデカ出るタイトルみたいなもので、 その下に出てくる本文というか、詳細分はあまり気にしていなかった。

後々見ると"fix requested changes"とか"instead into"、一体何だということになりますね。

たとえPRの途中のコミットであっても、雑なコミットを入れず、ちゃんと考えてやりましょう。 といいつつも、これでも結構考えて入れてたほうで、どういったコミットメッセージにするかで 大分悩みました。実際、要求されたコードを変更したわけですし。

この件での反省は

  1. コミットメッセージはあとから読めることがどこであっても重要
  2. たとえsquashでマージされて目につかなくとも。
  3. 普段から英語でコミットの説明をできるようになっておくべき。

もちろん、これらはそれぞれのマージのされ方によって違ってくるでしょう。 今回は特に最初から気をつけていれば生まれなかったコミットなので、 "PR"というモノへの臨み方というか、コミットというものを改めて考えさせられました。

それはそれはコミットメッセージをどうするか、という議論が絶えないわけですね。

まとめ

結果的には無事マージされたはじめてのPR、いろいろ反省点がありましたが、 こうして反省文を書いて次こそは!次こそはデキるPRを出すんだぞ!!ということで 今回はここで終了です。MammutのownerであるAaronepowerさん、 的確なレビューとコメントサポート本当にありがとうございました。

ここで言っても仕方ないんだけどね。

大阪丼にプルリク投げた件 その2 まとまりのない感想文