Skip to content

Conversation

@Morriar
Copy link
Collaborator

@Morriar Morriar commented Mar 28, 2025

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 the T.let if any.

So this:

A = 42
B = T.let(42, Integer)
C = foo(T.let(42, Integer))

gives this:

A = T.let(T.unsafe(nil), T.untyped)
B = T.let(T.unsafe(nil), Integer)
C = T.let(T.unsafe(nil), T.untyped)

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 🤔

@Morriar Morriar added breaking-change Change breaking retro-compatibility chore Chore task labels Mar 28, 2025
@Morriar Morriar self-assigned this Mar 28, 2025
@Morriar Morriar requested a review from a team as a code owner March 28, 2025 14:41
@Morriar Morriar requested a review from paracycle March 28, 2025 14:43
tree = parse_rbi(rbi)
assert_equal(rbi, tree.string)
assert_equal(<<~RBI, tree.string)
Foo = T.let(T.unsafe(nil), T.untyped)
Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Collaborator Author

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.

@st0012
Copy link
Member

st0012 commented Mar 28, 2025

Given it's a breaking change, can the description also include the benefits it will bring too?

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

I don't think this is a breaking change at all. The fact that we were including actual constant values in RBI files wasn't intentional and there should be nothing that relies on then being there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Change breaking retro-compatibility chore Chore task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants