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

Refactor and improve NamedTuple deserialization from JSON and YAML #12008

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 18, 2022

  • Makes the temporary value variables non-nilable unless their corresponding types are nilable. This saves the downcasts at the end of deserialization.
  • Fixes compilation error for deserialization of NamedTuples that use file-private value types.
  • Fixes compilation error for deserialization of empty NamedTuples.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:refactor topic:stdlib:collection topic:stdlib:serialization labels Apr 18, 2022
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I'm wondering if it wouldn't be easier to always use an uninitailized value and keep track if it was found. And only apply nil handling for nilable properties when constructing the final tuple.

Comment on lines 273 to 276
%var{key.id} = uninitialized typeof(begin
value = uninitialized self
value[{{ key.symbolize }}]
end)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this work?

Suggested change
%var{key.id} = uninitialized typeof(begin
value = uninitialized self
value[{{ key.symbolize }}]
end)
%var{key.id} = uninitialized {{ type }}

Copy link
Member

Choose a reason for hiding this comment

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

I think we should put a comment around why we do it this way. I think I made the exact same comment in a previous PR but now I can't remember why doing it the way @straight-shoota (and previously me) suggest is not a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's #11834. But there is an even more idiomatic way of expressing this: #12011

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @HertzDevil 🙏

@straight-shoota straight-shoota added this to the 1.6.0 milestone Jun 26, 2022
@straight-shoota straight-shoota merged commit f11f9d2 into crystal-lang:master Jul 14, 2022
@HertzDevil HertzDevil deleted the bug/namedtuple-from_json branch July 15, 2022 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:refactor topic:stdlib:collection topic:stdlib:serialization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants