-
Notifications
You must be signed in to change notification settings - Fork 10
Const holds a type rather than a value #445
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandre Terrasa <[email protected]>
A::B::C = Foo | ||
RBI | ||
|
||
tree = parse_rbi(rbi) | ||
assert_equal(rbi, tree.string) | ||
assert_equal(<<~RBI, tree.string) | ||
Foo = T.let(T.unsafe(nil), T.untyped) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider RBI files as documentation? Could this be a confusing change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, will this create huge RBI changes when it's adopted by Tapioca?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we consider RBI files as documentation? Could this be a confusing change?
From RBI files what we really care about is the type (and accessorily the comment since we also extract it). I'm not sure the value as that much importance when it comes to document behaviour especially if the value is not a literal.
will this create huge RBI changes when it's adopted by Tapioca?
If you mean diff, yes. If you means API change for Tapioca to use it? Less.
Given it's a breaking change, can the description also include the benefits it will bring too? |
Follow up on #408 (comment).
In the context of an RBI file, does it really make sense to hold the value of constants?
This PR proposes to change
Const
so it only stores the type we find a theT.let
if any.So this:
gives this:
This is quite a breaking change for the gem's API and should be released with at least a minor version bump.
I think we can apply the same logic to optional positional and keyword parameters 🤔