詩と創作・思索のひろば

ドキドキギュンギュンダイアリーです!!!

Fork me on GitHub

コードレビューのときに見ているところ

あるときコードレビューするときにどういうところ見てるんですか？ と訊かれてたしかに自分でもあまり言語化したことはなかったな、と気づいたので簡単に書いておく。

変更意図が要求に沿っているか

  • そもそも実現しようとしていることが、ユーザやプロダクトオーナーの要求に沿っているか。モデリングや実装のコンテキストを自分でも把握しておく。
  • 関連する別の変更やイシューなど、自分が知っていて相手が知らない有意義な情報があったらコメントする。

モデリングが妥当か

  • モデルによって意図が表現できているか。仕事が適切な粒度で明確に切り分けられているか。意図のない共通化がなされていないか。
  • わかりやすい名前がつけられているか。ここが混乱していると何かがよくないサイン。既存のコードがすでに……ということもある。そういう場合は改善できそうな道筋について議論できるとベター。
  • 仕事にあったインタフェースになっているか。テストを書いたときに素直なコードになっていると望ましい。例外の取り扱いや、アウトプット先を変えたときに無理なく使えそうか、とかも考える。

実装が妥当か

  • プロダクションでの性能が意識されているか。逆に無意味に性能を追ってコードが読みづらくなっていないか。
  • アルゴリズムに誤りがないか。とくに外部・外界とのやりとりを伴う場合、エッジケースがテストされているか。
  • 異常が発生したときにデバッグ・調査しやすいか。エラー（ログ）に必要な情報が含まれているか。
  • 既存のコードが今回の意図に合わせて変更されているか。

読み手の負荷が高くないか

  • コメントが書かれているか。ドキュメントやドキュメントへのリンクがあるか。
  • 読みづらいコードになっていないか。やむなくそうなっている場合はコメントがあるか。Pull Request のコメントで補足される場合があるけど、そういうときはコメントに書いてもらうようにする。
  • 議論の最中で出てきた未来の仕事が TODO とか FIXME となってコメントに残されているか。
  • この言語では/チームでは/プロダクトではしない書き方がないか。このへんは完全に知識の問題で、逆に教えてもらうチャンスでもある。機械がやってくれてもいいところ。

ざっと書き出したものなのでこれが全てではないと思います。

意図を理解したら、変更を順に見ていき、必要に応じて実行フローの前後を確認して、違和感があったり理解できないところにコメントをつけていくような流れ。

また、指摘や質問するだけではなく「ここはこう読み取りました」というコメントを残すこともある。これは認識を揃えるため。コードレビュー初心者で何をしたらいいかわからない、という人はこの「俺はこう思ったっす」を書いていくところから始めたらよいと思う。

gotest.tools でテストスナップショットをインラインに記録する

テキストや何らかのデータ構造（プログラムを含む）を生成するタイプのコードを書いているときはスナップショットテストが便利で、

  1. とりあえず一度スナップショットを生成してみる
  2. 生成されたものを人間が検分する
  3. 以降はスナップショットが壊れないように・意図したとおりに変更されるようにコードを書いていく

みたいなことをしている。このようにテストスナップショットには、「いったいこのコードで何がアウトプットされるのか？」に対するある種の自動ドキュメンテーションの機能がある。こういうものが欲しくなることは自分の書いたコードに限らずほかにもあって、たとえばクエリビルダのようなブラックボックスをテスト中で用いる際の sanity check 兼ドキュメントとしても有用。

まあ、ドキュメントとして使うことを意識するなら、（それが小さくすむなら）コードの近くにあったほうがありがたい。それでテストスナップショットをコード中に置くようなツールを書こうと思って調べてたら、すでにそういうものがあった。前にも紹介した gotest.tools というやつだ。

これには golden というスナップショットテストの仕組みがあり、testdata/ 以下に置いたファイルとの比較をしてくれるのだけど、それだけではなくわりと一般的に提供される assert.Equal のような関数にも実はスナップショット的に expected を更新してくれる機構がある。こちらはテスト中で値が比較されたとき、expectedXXX と名付けられた変数がその引数に含まれていたら、その変数自身を元のコードで書き換えてしまう、ということをやっている。なかなか大胆。

わかりづらいと思うので実際のコードで説明すると、

func TestBuild_Mutation(t *testing.T) {
    s, err := Build(...)

    expectedMutation := `mutation CreateReviewForEpisode($ep: Episode) {
  createReview(episode: $ep) {
    stars
    commentary
  }
}`
    assert.Equal(t, string(s), expectedMutation)
}

https://github.com/motemen/go-graphql-query/blob/7e5bb57182f75951f0bb5ce96293835637760d7e/query_test.go#L143-L170

以上のコードのうち expectedMutation := の文の右辺は go test -update によって生成されたものだ。こういった感じで、アウトプットが小さい場合や読み手にその場で説明したいような場合にはコード中に埋め込めると便利ですね。

もともとトップレベルの変数の更新しか対応してなかったのを PR 送って関数内の変数にも対応してもらった。まだタグが切られてないので、これを使うにはハッシュを指定して go get する必要がある。文字列以外のデータ構造にも対応できるといいんだけど、そこまでやるなら別ライブラリかな～。

SlackのログをBigQueryにインポートする（手動）

Admin 限定技だけど、Slack にはデータのエクスポートという機能があって、ワークスペースのパブリックチャンネルの会話データを zip でダウンロードすることができる。これがまあまあ便利な代物で、通常なら API を叩きまくらないと得られない諸々の情報を一括で手に入れることができる。

エクスポートした zip の中身はこんな感じになっている:

.
├── channels.json
├── integration_logs.json
├── users.json
├── <channel>
│   ├── yyyy-mm-dd.json
│   ├── ...
│   └── yyyy-mm-dd.json
└── <channel>...
    └── ...

この channels.json とかは conversations.list API の結果である……のかと思いきや、少しスキーマが違うようで、独自のものを返している模様。

こんな感じになっている:

[
{
    "id": "CXXXXXXXX",
    "name": "general",
    "created": ...
    "creator": "UXXXXXXXX",
    "is_archived": false,
    "is_general": true,
    "members": [
        "UXXXXXXXX",
        ...
    ],
    "topic": { ... },
    "purpose": { ... }
},
...
]

members なんかがあるのは特殊なんじゃなかろうか。yyyy-mm-dd.json も:

[
    {
        "client_msg_id": ...
        "type": "message",
        "text": "うんこ",
        "user": "UXXXXXXXX",
        "ts": "##########.######",
        "team": "TXXXXXXXX",
        "user_team": "TXXXXXXXX",
        "source_team": "TXXXXXXXX",
        "user_profile": { ... },
        "blocks": [ ... ],
        "reactions": [
            {
                "name": "hankey",
                "users": [ ... ],
                "count": 1
            },
            ...
        ]
    },
    ...
]

といった感じで conversations.history より情報が多め。ちょっと余談だけど、情報多めな割にこの JSON にチャンネル名が（ID も）含まれていない。ファイルパスに入っているからそれでわかるだろうという一種の正規化なのかもしれないけど、このおかげで面倒なシェルスクリプトを書くことになった。メタ情報による正規化はあまりうれしくないことがわかる。

さて、この reactions というデータが興味深い。誰がどんなリアクションをつけたか、がわかる（API ならいちいち reactions.get することで得られるデータ）。

そういうわけで、今回はこのエクスポートされた zip を BigQuery にインポートしようと思います。Embulk を使うことにする。できあがりは以下のリポジトリ。

GitHub - motemen/example-slack-logs-ingest-bigquery

README にあるとおり、

  • ./data にエクスポートした zip を置いて、
  • ./config/_out.yml.liquid を書いて、
  • docker compose run --rm embulk bash -c 'embulk run /config/embulk.yml.liquid'

で BigQuery へのインポートが行える。

できたテーブルに以下のようなクエリを叩くと、リアクションランキングなどが得られる。おもしろいですね。まあ text 本文などを置くのははばかられると思うので、転送しないほうがよいかも。

WITH messages_flat_reactions AS (
SELECT
  *,
  JSON_VALUE(reaction_item, "$.name") AS reaction_name,
  JSON_VALUE(reaction_item, "$.count") AS reaction_count
FROM
  `xxx.slack_messages`
LEFT JOIN
  UNNEST(JSON_QUERY_ARRAY(reactions)) AS reaction_item
WHERE
  reactions IS NOT NULL
)

SELECT
  timestamp, channel, text, reaction_name, reaction_count
FROM messages_flat_reactions
ORDER BY
  reaction_count DESC
LIMIT 10

Embulk について

Embulk を真面目に使ってみたのは初めてだったけど、いくつかハマりポイントがあった。

  • embulk の jar ファイルはシェルスクリプトとしても解釈できるように工夫されているので、公式サイトでは jar を chmod +x して PATH に入れるように指示されている。手元ではこれで動くが、Docker で動かすときに直接 ENTRYPOINT にしてしまうと動かない。bash などを噛ませて動かす必要がある。（何も考えずに java -jar を指定するとメモリ不足になりがち）
  • embulk-output-bigquery は依存ライブラリが Ruby >= 2.6 を求めがち（https://github.com/embulk/embulk-output-bigquery/issues/144）な一方 embulk 同梱の JRuby はちょっと古いので、ひとつひとつ pin しておく必要がある。これプラグイン側でなんとかできないかなあ。Embulk v0.11 では JRuby のバージョンを自分で選べる らしいので、それ待ちなのかもしれない。

はてなで一緒に働きませんか？