Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mame
Copy link
Member

@mame mame commented May 17, 2023

No description provided.

@yui-knk
Copy link
Collaborator

yui-knk commented May 17, 2023

Just curiosity, how did you generated these files?

VERSION: String
include Comparable

def to_s: -> String
Copy link
Collaborator

@yui-knk yui-knk May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect these methods comes from Lrama::Rule, which is a struct,

def to_s

Is this intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is known issue ruby/typeprof#84

attr_writer accept_symbol: bot
def term?: -> untyped
def nterm?: -> untyped
def eof_symbol?: -> false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May the type be bool?

@mame
Copy link
Member Author

mame commented May 17, 2023

Just curiosity, how did you generated these files?

I did:

$ typeprof lib/lrama/*

@Little-Rubyist
Copy link
Contributor

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

  • PRマージ前にSteepfileの check lib/lrama/bitmap.rbcheck lib/lrama にしてほしいです.その上で steep check が通るようにしてください.
  • 今回の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を読むと良いです.

This was referenced May 18, 2023
@yui-knk
Copy link
Collaborator

yui-knk commented May 19, 2023

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だと思います

@akouryy akouryy mentioned this pull request May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants