Migrate all T::Struct usages to bare Ruby classes#841
Conversation
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Morriar
force-pushed
the
at-remove-t-struct
branch
from
e91064c to
90bd8f6
Compare
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>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Morriar
force-pushed
the
at-remove-t-struct
branch
from
90bd8f6 to
80a1d36
Compare
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
amomchilov
left a comment
amomchilov
left a comment
There was a problem hiding this comment.
I guess if you migrated to Data.define instead, you'd need a whole bunch of RBI files just to specify the sigs, right?
amomchilov
changed the title
T::Struct usages to bare Ruby classes
Indeed, Sorbet doesn't have a way to associate types to data fields yet. |
paracycle
left a comment
paracycle
left a comment
There was a problem hiding this comment.
I have a few minor comments, but looks good to me
| #: String | ||
| attr_reader :uri | ||
|
|
||
| #: Range |
There was a problem hiding this comment.
This is quite confusing. Even though it probably resolves to LSP::Range, I still find it confusing to see a bare Range reference, and I assume it would be a reference to ::Range and not something else. Should we keep the namespace here?
| const :code, Integer | ||
| const :message, String | ||
| const :information, Object | ||
| #: Range |
| #: Location? | ||
| attr_reader :location | ||
|
|
||
| #: Range? |
There was a problem hiding this comment.
Same comment here, but I think the old code was similarly confusing.
| #: (SymbolPrinter printer) -> void | ||
| def accept_printer(printer) | ||
| h = serialize.hash | ||
| h = hash |
There was a problem hiding this comment.
Interesting! I am noticing this code now and I am not sure if this will always work the way you intend it to. The return value of hash isn't necessarily unique, so you can't rely on it (solely) for Set inclusion.
a.eql?(b)impliesa.hash == b.hash
from https://docs.ruby-lang.org/en/3.4/Object.html#method-i-hash
but the converse isn't necessarily true, i.e. a.hash == b.hash does not necessarily imply a.eql?(b).
3 participants
Also enable
Sorbet/ForbidTStructcop after that.