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

Add a get_or_add method to Dictionary #78095

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jun 10, 2023

Implements this proposal and closes godotengine/godot-proposals#7059

Copy link
Member

@ajreckof ajreckof left a comment

Choose a reason for hiding this comment

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

The implementation is quite simple and in line with other dictionary functions. I'm just not sure about the name but I can't find a better one and python setDefault is even worse so this might be the best name😓

@RandomShaper
Copy link
Member

My issue with get_or_set_default() is that it can be read as "get or set-default" and "get-or-set default." I'd say it's intended to be the former. get_ensuring_default() sounds better to me.

@aaronfranke
Copy link
Member Author

aaronfranke commented Jun 15, 2023

I am good with either of the proposed names, none seems obviously better to me.

Really, it's more like "(get) or ((set and get) default)".

@RandomShaper
Copy link
Member

Don't you like get_ensuring_default() more the more you read it? 😁

@AThousandShips
Copy link
Member

I think get_ensuring_default() risks seeming like it always sets it, unsure how to state it clearly

@RandomShaper
Copy link
Member

At this point I don't think the full behavior of the method can be summarized enough without leaving some thought to the user. In the defense of get_ensuring_default() I'll say that the "ensuring default," idea can't really mean anything else than "return if the key exists, otherwise set first," because there's already set().

@Repiteo
Copy link
Contributor

Repiteo commented Jun 20, 2023

What about ensure_key_then_get()? This makes the order of operations clear, as you're retrieving the value at the specified key regardless; if the key already exists, you just get the stored value — if the key didn't exist, you ensure it does via a default value, then you retrieve said default value

@Iniquitatis
Copy link
Contributor

How about get_or_add or something along these lines?

@RandomShaper
Copy link
Member

How about get_or_add or something along these lines?

I support that. Just let's be sure that Dictionary's jargon has room for "add"; in other words, it's not already using "insert" or the like for the same meaning.

@ajreckof
Copy link
Member

ajreckof commented Jun 27, 2023

No function use either of them yet and doc use both even in the same sentence like so :
image
So I would say either add or insert is okay I'm more accustomed to insert for dictionaries but this might just be a personal thing

@RandomShaper
Copy link
Member

RandomShaper commented Jun 27, 2023

Maybe "insert" is more used in Godot containers in general.

get_or_insert() then?

@SysError99
Copy link
Contributor

We should instead use what similar languages were offering, in this case set_default is a great candidate since it's very similar to Python.

https://docs.python.org/2/library/stdtypes.html

If this PR gets merged successfully, I'll try to cherry pick this to 3.x

@ajreckof
Copy link
Member

I really don't like the Python set_default as it is really unclear on what it does

@aaronfranke aaronfranke force-pushed the dict-get-or-set-default branch from 32a5f39 to f238cc2 Compare August 1, 2023 19:14
@aaronfranke aaronfranke force-pushed the dict-get-or-set-default branch from f238cc2 to ad22506 Compare September 4, 2023 05:04
@aaronfranke aaronfranke force-pushed the dict-get-or-set-default branch from ad22506 to 537f5c1 Compare September 24, 2023 17:53
@aaronfranke aaronfranke modified the milestones: 4.2, 4.3 Oct 11, 2023
@peterhoglund
Copy link

I read get_or_set_deafault as getting or setting the default. I also agree Python's set_default is bad since it doesn't explain what is happening. What about just get_or_set?

@aaronfranke
Copy link
Member Author

@RandomShaper "Insert" is usually associated with some kind of order, like with Array.insert(), so I would prefer to not use that terminology here. This method does not affect the order, if anything is added then it always goes to the "end".

@RandomShaper
Copy link
Member

Back to get_or_add(), then?

@peterhoglund
Copy link

Sorry, I didn't see get_or_add() was suggested. I like it since it's obvious what it does. get_or_insert() is equally good imo, but since get_or_add is shorter it wins for me.

@aaronfranke aaronfranke force-pushed the dict-get-or-set-default branch from 537f5c1 to 65e5156 Compare December 6, 2023 05:21
@akien-mga
Copy link
Member

I think there's a relatively good consensus for get_or_add, so I suggest changing this PR to use this name.

@aaronfranke aaronfranke force-pushed the dict-get-or-set-default branch from 65e5156 to 8b7d99e Compare December 6, 2023 17:29
@akien-mga akien-mga changed the title Add a get_or_set_default method to Dictionary Add a get_or_add method to Dictionary Dec 6, 2023
@akien-mga
Copy link
Member

The commit message also needs to be amended.

@aaronfranke aaronfranke force-pushed the dict-get-or-set-default branch from 8b7d99e to 437586b Compare December 6, 2023 17:44
@YuriSizov YuriSizov merged commit d02b368 into godotengine:master Dec 16, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@aaronfranke aaronfranke deleted the dict-get-or-set-default branch December 24, 2023 23:02
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.

Add a get_or_set_default method to Dictionary
10 participants