Skip to content
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

Fix FixedHash serialization #327

Merged
merged 1 commit into from
May 25, 2021
Merged

Conversation

paracycle
Copy link
Member

Motivation

T::Types::FixedHash did not used to differentiate between the key types and serialized all keys as Symbols. This created an information loss when Tapioca was serializing parameters in signatures that had fixed hash values with String or mixed keys.

Implementation

The same change is done upstream in Sorbet in sorbet/sorbet#4223 and this is a backport of that change to older versions of Sorbet.

Tests

Added an explicit test for this case.

@paracycle paracycle requested a review from a team May 25, 2021 12:47
@paracycle paracycle force-pushed the uk-fix-fixed-hash-serialization branch from 6c1f1be to 17c64ed Compare May 25, 2021 12:48
`T::Types::FixedHash` did not used to differentiate between the key
types and serialized all keys as Symbols. This created an information
loss when Tapioca was serializing parameters in signatures that had
fixed hash values with String or mixed keys.

The same change is done upstream in Sorbet in sorbet/sorbet#4223
and this is a backport of that change to older versions of Sorbet.
@paracycle paracycle force-pushed the uk-fix-fixed-hash-serialization branch from 17c64ed to 60b46db Compare May 25, 2021 12:49
@paracycle paracycle merged commit e52fbd0 into master May 25, 2021
@paracycle paracycle deleted the uk-fix-fixed-hash-serialization branch May 25, 2021 16:02
@shopify-shipit shopify-shipit bot temporarily deployed to production May 25, 2021 22:53 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants