lrama icon indicating copy to clipboard operation
lrama copied to clipboard

Set up rbs_collection.yaml and add a TypeProf-generated prototype rbs

Open mame opened this issue 2 years ago • 4 comments

mame avatar May 17 '23 05:05 mame

Just curiosity, how did you generated these files?

yui-knk avatar May 17 '23 07:05 yui-knk

Just curiosity, how did you generated these files?

I did:

$ typeprof lib/lrama/*

mame avatar May 17 '23 08:05 mame

https://cookpad.connpass.com/event/282436/ で話していた内容です.(sorry I'll write Japanese...)

  • PRマージ前にSteepfileの check lib/lrama/bitmap.rbcheck lib/lrama にしてほしいです.その上で steep check が通るようにしてください.
    • https://github.com/ruby/lrama/blob/718cbb35608e7f0d737c961ebba5fae2c7fd6ff8/Steepfile#L6
  • 今回のgenerated-rbsを導入後,手作業で型を sig/lrama/ 以下に追加していく場合は rbs subtract sig/lrama/typeprof-generated.rbs sig/lrama/#{added_rbs_file.rbs} などで除去をしてもらえると嬉しいです.
    • 要は sig/lrama/*.rbsを追加された定義はgeneratedからどういう形にせよ消えてほしいです.
    • 参考:rbs subtract のドキュメントはPR内にあるhackmdを読むと良いです.

Little-Rubyist avatar May 18 '23 13:05 Little-Rubyist

PR & コメントありがとうございます。その後のmameさんcommitを踏まえたうえでの感想ですが

  • raise を書いてまわるのはちょっと入れるのに厳しいかなという気がします
    • 気合で全部レビューするのは不可能ではないけど厳しい
    • できればraiseしなくていい方法がほしい
  • 今後個別メソッドにrbsを付与するときにコード側の修正を同時にいれるのは大丈夫です、メソッド単位であればそこまでレビューも大変じゃないはず(もちろん変更量によりますが)
  • rbs_collection.yaml を入れるのは必要なので別のPRをつくっていただいてやるのがいいとおもいます
  • sig/lrama/typeprof-generated.rbs についてはsteep checkの対象には含めないほうがいいと思います。ドキュメントとして、もしくは未定義の型の一覧としてtypeprof-generatedを置いておく のはよいと思います。そのうえで sig/lrama/ に今後ファイルrbs定義がふえたときに typeprof-generated.rbs から消し忘れないような仕組みをCIとしていれられるとbestだと思います

yui-knk avatar May 19 '23 01:05 yui-knk