golangci-lint の enable-all で コーディングルールを明確にする試み

golangci-lint の enable-all で コーディングルールを明確にする試み なかひこくん(@takanakahiko) @Asakusa.go #2

誰？ なかひこくん @takanakahiko 新横浜(実は浅草の飛び地なんです！)に住んでいます

type Runer interface { Run() } さっそく 皆さんに質問 Human が Runer を実装して いることを確認したいです。 皆さんならどっちの書き方にし ますか？ type Human struct { name string } func (h *Human) Run() { ... } // 上記の Human が Runer を実装していることを確認する // 皆さんはどっちで書きますか？ var _ Runer = &Human{} // ① var _ Runer = (*Human)(nil) // ②

自分としては - どっちでも問題はない(メリットやデメリットはあるが)と思う - ただしどちらかに統一されていると必要はある(これが大事) こういうのとても多いですよね。 補足: go.dev/doc ではポインタなら②が例示はされている https://go.dev/doc/faq#guarantee_satisfies_interface

• https://go.dev/doc/faq#guarantee_satisfies_interface

チームで書くときは？ チームではこういうのを決めて統一する必要がある - 結局どちらに統一するのか？ - これを決めるための何かしらの方針が欲しい - なぜそちらの書き方で書くのか？ - この理由を明確にしておきたい（とりあえず？明確に必然性があるから？） - どう徹底(統一)する？ - レビュワーが指摘する？でもそれって仕組み化したいよね なんだかんだこういうのが大変なんですよね

こういう時に役立つのが golangci-lint - go向けのlinter詰め合わせセット - サードパーティで開発されたやつを取り込んでいる形 - 利用者側は .golangci.yml で利用する linter を選択する - 君だけの最強の .golangci.yml で勝利を勝ち取れ！

こんな感じで叱ってくれる。 色々なlinterから叱られてるのがわかると思う。

type Runer interface { Run() } (再掲) 皆さんに質問 Human が Runer を実装して いることを確認したいです。 皆さんならどっちの書き方にし ますか？ type Human struct { name string } func (h *Human) Run() { ... } // 上記の Human が Runer を実装していることを確認する // 皆さんはどっちで書きますか？ var _ Runer = &Human{} // ① var _ Runer = (*Human)(nil) // ②

# .golangci.yml linters: # enable-all で全てのlinterを有効化 enable-all: true そこで golangci-lint enable-all を使ってみる。 すると何やら怒られる。

こうやって対応を決めることができる main.Human is missing field name (exhaustruct) → なるほど！① var _ Runer = &Human{} だとstruct fieldの設定漏れを許 容するようなソースコードになるから指摘されるんだね → struct fieldの設定漏れは検知できる仕組みが欲しいし、それに変な 例外は作りたくなので② var _ Runer = (*Human)(nil) に統一しよう

なぜ enable-all ？ - やらないことを決める方が負荷が少ない - やらないことの理由を挙げる方が明確にしやすい - linterを逐次設定する方針だと「採用しない理由」がわかりづらい - disable-all かつ enable で逐次指定する形 - 「イケているlinterが採用されていない！」→「考慮漏れ？」「後回しになっているだ け？」「理由があって対応していない？」と分かりづらい

この運用をするときの .golangci-lint.yml の例 # https://github.com/takanakahiko/discord-tts/blob/2195764ba46d1d6b251cea4f29b0cd9e6839d824/.golangci.yml linters: enable-all: true disable: - wsl # 余計な改行をなるべく含まないようにすることで得られる見通しの良さを重視するため - nlreturn # 上記と同様 - gosmopolitan # 現在はi18n/l10nを検討していないため - depguard # 規模的に依存関係の流れを厳格に管理する必要性はないため - forbidigo # いまのところ特に禁止したい表現はないため - gomnd # The linter 'gomnd' is deprecated (大袈裟だが) - execinquery # The linter 'execinquery' is deprecated 理由をつけて disableをする # 続く

• https://github.com/takanakahiko/discord-tts/blob/2195764ba46d1d6b251cea4f29b0cd9e6839d824/.golangci.yml

この運用をするときの .golangci-lint.yml の例(2) # 続き linters-settings: varnamelen: ignore-decls: - v *discordgo.VoiceStateUpdate # パッケージの使用例がその命名であるため - m *discordgo.MessageCreate # パッケージの使用例がその命名であるため revive: rules: - name: unexported-return disabled: true # ireturnへの対応を優先するため # 続く linterの個別設定の カスタマイズにも理由があると嬉しい

この運用をするときの .golangci-lint.yml の例(3) # 続き linters-settings: funlen: lines: 100 # デフォルトの60だと余計な関数の分割が発生するため statements: 60 # デフォルトの40だと余計な関数の分割が発生するため gomoddirectives: replace-allow-list: - github.com/jonas747/dca # 該当パッケージが壊れているので独自にパッチを当てたものを利用したいため cyclop: max-complexity: 24 # デフォルトの10だと余計な関数の分割が発生するため 現状このリポジトリの最大値は 23なので24を設定。(この場合は「 23になるのは仕方がないよね」という共通認識を作れているはず ) こう設定することで、 complexityの最大値を更新するような次の変更があったらそのタイミングで議論の機会が生まれる。

注意点 - .golangci.yml の編集のハードルは極力下げるように努力すべき - 必要以上にコーディングに制約を作ることが目的ではない - 「いまこうなっている理由」を明確にしているだけ - 「いまこうなっている理由」より優先すべきものがあったらガンガン変更する - 「今考えたものが最高で絶対的なルール」なんてあり得ない → 後から入る人がその人 たちにとって心地よいルールにしてくれるという信頼をチームで醸成する - disabeの理由として「あとで対応する」もガンガン許容する - 「あとで対応する」ことが分かっているなら目的は達成できている - 「なぜ対応されていないのか分からない」が一番避けるべき状態

デメリット(と感じられそうなもの) - golangci-lint のアップデート時にlinterが増えるのでメンテが大変 - だが、その時に方針をチームメンバーで話し合えるのは良いことだと思う - 言語のバージョンをあげてから暫くして上げるのがおすすめ(linterが対応する期間 を待つ) - 明らかに不要なlinterにも理由をつけてdisableする必要がある - でも「明らかに」って自分の感想でしかないので、やっぱり明文化は必要だと自分は 思います - 言語化することでジュニアに「当たり前」の感覚を共有するのも大事

まとめ - golangci-lint で enable-all をすると表記のブレが減らせる - 取り上げていないlinterを理由付きで無効化することができる - 「対応していない理由」が明確になる！ - (アップデート時の .golanci.yml のメンテナンスが少し大変) - みんなどうやってるかハッシュタグとかで教えて欲しい〜