-
-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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 parsing Resource type as value type of a Dictionary #100933
Conversation
Hopefully this gets merged soon. My friend needs to finish his godot project for skool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice to add tests for typed arrays and dictionaries, somewhere here:
godot/tests/core/variant/test_variant.h
Line 1740 in aa65940
TEST_CASE("[Variant] Writer and parser array") { |
godot/tests/core/variant/test_variant.h
Line 1790 in aa65940
TEST_CASE("[Variant] Writer and parser dictionary") { |
Also please correct the commit message. |
4b0fbe4
to
2c1b245
Compare
Should this be added in this PR, or a future one? I suppose the Dictionary one fits here, since this PR fixes a bug that slipped through as a result of not having this check, but it may be out of scope for the Array one. |
2c1b245
to
b0845da
Compare
I feel like the code could potentially be cleaned up/optimized, though I am not sure what usecase the parentheses after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at surrounding code, that seems to make sense, but I'm not expert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye!
Thanks! |
Fixes #100889.
A line was forgotten, so I added it. I do not believe it affects anything else as the scope is specifically for this use case.