Simplify implementation of Serializable#initialize
#13433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kostya
reviewed
May 5, 2023
| {% else %} | ||
| uninitialized ::Union({{value[:type]}}) | ||
| {% end %} | ||
| %var{name} = uninitialized ::Union({{value[:type]}}) |
There was a problem hiding this comment.
interesting why this condition was needed?
There was a problem hiding this comment.
The convoluted implementation of the finalizing steps (where the ivar is assigned) always assigned @{{name}} = %var{name} if value[:nilable]. So it needed a value and could not be uninitialized.
There was a problem hiding this comment.
With this patch, it only assigns if %found{name} so there's no need for a default initialization.
There was a problem hiding this comment.
Before #13238 %var{name} was always initialized to nil and then that nil type was filtered out later when the ivar was not nilable. uninitialized is a much better tool, though. It doesn't add Nil to the type.
HertzDevil
approved these changes
May 5, 2023
straight-shoota
changed the title
Serializable.newSerializable#initialize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The deserialization logic in
JSON::SerializableandYAML::Serializableis unnecessarily complex. This patch is a refactor for simplicity.The code for handling whether a value was (not) found and the different conditions (nilable, default) was a major jumble.
The special handling for nilable types is completely unnecessary. If we remove that special case, it all becomes much more straightforward. It also gets rid of duplications. This is the main point of this PR, addressed in the second commit.
The first commit adds specs for #13431 to avoid breaking that in the refactor.
The third commit doesn't change anything structurally, it just simplifies the macro code with a different mechanism. The conditional nesting of
read_null_or/parse_null_oradds unnecessary complexity. We can omit that by just asking the parser for a null value and acting accordingly.