Skip to content

Add missing initializer_list constructor to TypedDictionary#104664

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
tomfull123:missing-typed-dictionary-initializer-list
Apr 3, 2025
Merged

Add missing initializer_list constructor to TypedDictionary#104664
Repiteo merged 1 commit into
godotengine:masterfrom
tomfull123:missing-typed-dictionary-initializer-list

Conversation

@tomfull123
Copy link
Copy Markdown
Contributor

@tomfull123 tomfull123 commented Mar 26, 2025

Noticed while adding initializer_list support to godot-cpp godotengine/godot-cpp#1750

This PR adds initializer list support to dictionarys for object and refcounted types, less useful in Godot, but will be very useful in gdextensions, it just needs to get merged here first.

After this PR goes in, I also want to add support for object and refcounted types as dictionary keys, this PR currently only supports them as values.

@tomfull123 tomfull123 requested a review from a team as a code owner March 26, 2025 19:04
@akien-mga akien-mga changed the title Add missing initializer_list constructor for TypedDictionary Add missing initializer_list constructor for TypedDictionary Mar 26, 2025
@akien-mga akien-mga added this to the 4.x milestone Mar 26, 2025
@tomfull123
Copy link
Copy Markdown
Contributor Author

Originally added in #99751
Looks like it was just overlooked?

@akien-mga akien-mga requested review from Repiteo and bruvzg March 26, 2025 23:02
@tomfull123 tomfull123 changed the title Add missing initializer_list constructor for TypedDictionary Add missing initializer_list constructor to TypedDictionary Mar 27, 2025
@AThousandShips
Copy link
Copy Markdown
Member

We should probably add tests for this like there are already for standard Dictionary, to ensure it works as expected

@tomfull123 tomfull123 requested a review from a team as a code owner March 27, 2025 23:06
@tomfull123
Copy link
Copy Markdown
Contributor Author

We should probably add tests for this like there are already for standard Dictionary, to ensure it works as expected

Added some tests

@tomfull123 tomfull123 force-pushed the missing-typed-dictionary-initializer-list branch from c520a37 to df97060 Compare April 1, 2025 20:03
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Apr 2, 2025

There doesn't need to be a separate typed dictionary test file; typed tests already happen in the normal dictionary file, so those can be relocated there instead

Comment thread core/variant/typed_dictionary.h Outdated
@tomfull123 tomfull123 force-pushed the missing-typed-dictionary-initializer-list branch from df97060 to 9c5b8a6 Compare April 2, 2025 21:58
@tomfull123
Copy link
Copy Markdown
Contributor Author

There doesn't need to be a separate typed dictionary test file; typed tests already happen in the normal dictionary file, so those can be relocated there instead

I thought it would be nicer to have tests for each class in their own test file, but no worries, I've moved them into the dictionary test file

@tomfull123 tomfull123 requested a review from bruvzg April 2, 2025 22:00
Copy link
Copy Markdown
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code and tests look good to me.

@tomfull123 tomfull123 force-pushed the missing-typed-dictionary-initializer-list branch from 9c5b8a6 to 8a3f984 Compare April 2, 2025 23:18
Copy link
Copy Markdown
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.

Looks good!

@Repiteo Repiteo modified the milestones: 4.x, 4.5 Apr 3, 2025
@Repiteo Repiteo merged commit 21db848 into godotengine:master Apr 3, 2025
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Apr 3, 2025

Thanks! Congratulations on your first merged contribution! 🎉

@tomfull123 tomfull123 deleted the missing-typed-dictionary-initializer-list branch April 4, 2025 18:17
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.

6 participants