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 parsing Resource type as value type of a Dictionary #100933

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

Synzorasize
Copy link
Contributor

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.

@Synzorasize Synzorasize requested a review from a team as a code owner December 30, 2024 15:41
@Synzorasize Synzorasize changed the title Fix #100889 Fix parsing Resource type as value type of a Dictionary Dec 30, 2024
@Chaosus Chaosus added this to the 4.4 milestone Dec 30, 2024
@wyvrtn
Copy link
Contributor

wyvrtn commented Jan 7, 2025

Hopefully this gets merged soon. My friend needs to finish his godot project for skool

Copy link
Member

@dalexeev dalexeev left a 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:

TEST_CASE("[Variant] Writer and parser array") {

TEST_CASE("[Variant] Writer and parser dictionary") {

core/variant/variant_parser.cpp Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member

dalexeev commented Jan 7, 2025

Also please correct the commit message.

@Synzorasize
Copy link
Contributor Author

It would also be nice to add tests for typed arrays and dictionaries, somewhere here

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.

@Synzorasize
Copy link
Contributor Author

I feel like the code could potentially be cleaned up/optimized, though I am not sure what usecase the parentheses after Resource is usually for, as I haven't recalled seeing them before in tres or tscn files. I have only seen usecases of ExtResource and SubResource with parentheses. Other than that, I think it might be best to merge this for now.

Copy link
Member

@akien-mga akien-mga left a 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.

Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Good eye!

@Repiteo Repiteo merged commit a0f10a2 into godotengine:master Feb 11, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 11, 2025

Thanks!

@Synzorasize Synzorasize deleted the fix_100889 branch February 11, 2025 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading resources containing typed Dictionarys with values of type Resource causes a parse error
6 participants