Rewrite RBI generation for gems #297
Conversation
Morriar
force-pushed
the
at-rbi-gen
branch
4 times, most recently
from
b2fbcc2
to
d850143
Compare
There was a problem hiding this comment.
Left a few comments. I haven't gone over symbol_generator.rb in detail in terms of equivalence but the tests provide confidence. I am okay with the formatting changes with the new builder such as no parentheses and single line definitions for empty classes/modules.
Rewriters and rbi tests look good to me.
| sig { override.params(v: Printer).void } | ||
| def accept_printer(v) | ||
| v.printl(visibility.to_s) | ||
| end |
There was a problem hiding this comment.
We are printing visibility information both alongside method definition and on its own in a class. Given that RBIs are also used as documentation in Jump to definition I'm curious if it'll be better to always add visibility information to the method definition and print alongside it. While never printing a Visibility node on its own for methods?
private def foo; end
vs
private
...
def foo; end
There was a problem hiding this comment.
Good point. I'm all in favor (it also removes the need of NestNonPublicMethods). Curious of @paracycle's opinion on this?
There was a problem hiding this comment.
Like @KaanOzkan said, it would definitely make the privacy level of the method more accessible, at the risk of making the RBI more verbose. I like the idea and we can have it as the default, but it would be nice to allow this to be configurable, if possible.
There was a problem hiding this comment.
I'll provide a follow up PR making this kind of option available from the CLI, wdyt?
| return if symbol_ignored?(name) | ||
| klass = class_of(value) | ||
| klass_name = name_of(klass) | ||
|
|
||
| return if klass_name&.start_with?("T::Types::", "T::Private::") | ||
|
|
||
| type_name = public_module?(klass) && klass_name || "T.untyped" | ||
| indented("#{name} = T.let(T.unsafe(nil), #{type_name})") | ||
| tree << RBI::Const.new(name, "T.let(T.unsafe(nil), #{type_name})") |
There was a problem hiding this comment.
Observation: It might be good to make the T.let(T.unsafe(nil), XXX) part encapsulated by introducing something like an RBI:ValueConst.new(name, type: type) thing.
There was a problem hiding this comment.
It's a good idea but I'm not convinced in the naming nor the API, do you mind if we wait the rewrite of the DSL part to get more use cases and figure what's best?
| end | ||
|
|
||
| sig { params(string: T.nilable(String)).void } | ||
| def printt(string = nil) |
There was a problem hiding this comment.
Can we use a more descriptive name? Maybe indented_print?
There was a problem hiding this comment.
I would prefer not.
I chose those names so it aligns nicely in the code:
v.indent
v.printt "some code"
v.printn "some other code"
v.printl "more code"
v.dedentI added a comment on each of them to explain the behaviour. Is it better?
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Motivation
This PR makes the RBI generation for gems more flexible by introducing a clean and extensible API.
Implementation
symbol_generatorI recommend reading the changes on the test file commit by commit.
Tests
See included tests.