-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add merged
method to allow returning Dictionary after merging
#65526
Add merged
method to allow returning Dictionary after merging
#65526
Conversation
Hm that's not a common pattern in our API. Usually if it returns something it would be |
Well I opened this based on how I use my old utility method (I had a GDScript implementation before it was in core). tbh the method would be as useful even if it returned a new Dictionary; I think I originally made it modify the Dictionary, because it's cheaper than copying it (and Dictionary doesn't have any methods that return a new Dictionary). Tweens have similar API (i.e. it allows chaining method calls on the same object), so it's not unheard of. I just wanted to replace the old method in my project with the new
Well if there is use, they could. |
7358b83
to
764c127
Compare
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.
I know this is literally two years old but I do agree with the sentiment of Akien of the past.
And I still think some sort of merge and return is useful. I could make it a new method. |
Mhm! |
Should it return a new Dictionary or is it ok if it modifies and returns the same one? |
Methods such as Vector2's |
764c127
to
43b2057
Compare
Pushed as a new method. |
43b2057
to
3502a40
Compare
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.
In my opinion the first parameter could be called with
, although unfortunately that'd not be consistent with merge
, so hmm.
doc/classes/Dictionary.xml
Outdated
<param index="0" name="dictionary" type="Dictionary" /> | ||
<param index="1" name="overwrite" type="bool" default="false" /> | ||
<description> | ||
Creates a new Dictionary, which is a duplicate of this Dictionary merged with the other [param dictionary]. Useful for quickly making Dictionaries with default values. |
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.
Creates a new Dictionary, which is a duplicate of this Dictionary merged with the other [param dictionary]. Useful for quickly making Dictionaries with default values. | |
Returns a copy of this dictionary merged with the dictionary [param dictionary]. Useful for quickly making dictionaries with default values. |
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 sounds awkward either way.
"with other dictionary dictionary
"
"with other dictionary with
"
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's about making it easier to translate, as someone did mention to avoid "punning" for that reason.
788770c
to
b1694e3
Compare
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.
b1694e3
to
d68b7c3
Compare
merged
method to allow returning Dictionary after merging
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.
This has actually been a-okay for a while, at least documentation wise. I just forgot to approve of it.
Now it's a matter of determining if this is desirable or not (probably yes).
d68b7c3
to
eb0a624
Compare
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.
Let's merge! *starts fusion dance*
Thanks! |
Dictionary
merge()
is void right now, but it could return itself, so you can chain it or easily use as return value etc.