Add return types for callbacks in Ripper::SexpBuilder #2315
Conversation
|
I wasn't quite sure how to test callbacks like these with your tooling - open to ideas! |
Also match tok type to token type in lexer doc example
|
|
||
| def on_CHAR: (untyped tok) -> untyped | ||
| def on_CHAR: (String tok) -> [Symbol, String, [Integer?, Integer?]] |
There was a problem hiding this comment.
These two patterns of callbacks are defined here:
https://github.com/ruby/ruby/blob/master/ext/ripper/lib/ripper/sexp.rb#L116-L131
|
This change is limited as I'm not terribly familiar with Ripper. These changes will help https://github.com/castwide/solargraph pass its own typechecking; here's the use it makes of Ripper: https://github.com/castwide/solargraph/blob/41b892397be99fd6b445d1e308a3f512c5ec32e9/lib/solargraph/parser/comment_ripper.rb#L4 cc: @aamine in case you have thoughts |
|
Should I push up additional fixes to RBS types? |
|
@apiology |
|
I'm sorry - I don't know how to test callbacks like these with your tooling. Do you have any ideas? A small example and I can fill in the rest. |
|
If it helps, I am successfully using my fork of this repo to typecheck this file: https://github.com/castwide/solargraph/blob/0e82bbdcc9da7343dd028373797f2fe0373dbf6a/lib/solargraph/parser/comment_ripper.rb |
There was a problem hiding this comment.
Since the return values of all callback methods are never used, untyped or void seems appropriate.
Callback methods are meant to be overridden in subclasses, so perhaps they shouldn't be defined at all.
I wrote the original signatures myself, but I honestly have no memory of it...
The return values actually are used - they determine what is returned by the overall parse() method - see https://github.com/ruby/ruby/blob/a4a60195502add094fb52a587411bbd0c19facce/ext/ripper/lib/ripper/filter.rb#L59 As you note, Ripper::SexpBuilder is used by subclassing. The return types indicate to the developer what they must return from the methods they write as well as what information they can gain from the superclass method's return value. Here is the code I would like to be well-typed: https://github.com/castwide/solargraph/blob/0e82bbdcc9da7343dd028373797f2fe0373dbf6a/lib/solargraph/parser/comment_ripper.rb - and here is how the resulting code is used: https://github.com/castwide/solargraph/blob/767a5fceb74935d03f6757711bdf610df3e6fd7a/lib/solargraph/parser/parser_gem/class_methods.rb#L17
I am very familiar with that feeling 😁 |
No description provided.