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

Method name bikeshedding #60

Closed
dminor opened this issue Oct 24, 2024 · 32 comments
Closed

Method name bikeshedding #60

dminor opened this issue Oct 24, 2024 · 32 comments

Comments

@dminor
Copy link
Collaborator

dminor commented Oct 24, 2024

The proposal would add two new methods to Map:

  • getOrInsert(key, default) - Return value corresponding to the key if present, otherwise insert the specified default value and return that.
  • getOrInsertComputed(key, callback) - Return value corresponding to the key if present, otherwise call the callback function to determine the default value to insert and return.

Those names are placeholders, I'm opening this issue so we can bikeshed the final names to be used.

@dminor
Copy link
Collaborator Author

dminor commented Oct 24, 2024

There was a previous conversation (see #29) about naming for the original design of this proposal.

@dminor dminor mentioned this issue Oct 24, 2024
@nex3
Copy link

nex3 commented Oct 25, 2024

In Dart this is called putIfAbsent(), although setIfAbsent() might be a better fit given Map.set().

@dminor
Copy link
Collaborator Author

dminor commented Oct 31, 2024

cc a few people whom I recall expressing an opinion on the naming at plenary: @ljharb @rkirsling @acutmore @rbuckton @bakkot @syg

@bakkot
Copy link
Contributor

bakkot commented Oct 31, 2024

I don't like setIfAbsent because it emphasizes the action of inserting, whereas the primary use for this is fetching an existing value.

getOrInsert is good, but I don't love getOrInsertComputed. Nothing better comes to mind, though.

@zloirock
Copy link
Contributor

Both current names LGTM - definitely better than was before since they perfectly describe their logic.

@acutmore
Copy link

+1 to prefixing with get.... To add further rationale, Map.prototype.set returns the map instance, not the inserted value.

@rkirsling
Copy link
Member

I might be inclined to suggest getWithDefault, since the one issue with getOrInsert is that we want to get either way, we just might need to insert first.

I suppose this would also allow us to rearrange ...Computed into getWithComputedDefault.

@acutmore
Copy link

acutmore commented Nov 1, 2024

I suspect some may interpret getWithDefault as a method that never mutates the map - the method form of map.get(key) ?? defaultV.

@bakkot
Copy link
Contributor

bakkot commented Nov 1, 2024

I might be inclined to suggest getWithDefault, since the one issue with getOrInsert is that we want to get either way, we just might need to insert first.

You can look at it that way, but you can also look at as the insert case just returning the inserted value.

@ljharb
Copy link
Member

ljharb commented Nov 1, 2024

Just to throw out an extreme alternative, insertIfMissingThenGet (I’m not actually suggesting we do this one, ofc)

@jhwheeler
Copy link

jhwheeler commented Nov 29, 2024

How about something as simple as upsert itself?

This was suggested in a comment from the original thread here.

I think there is a good reason the proposal itself is called upsert 😁

@acutmore
Copy link

The API no longer has the "update" aspect. Only lazy initialisation. Making the name "upsert" less applicable than at the start.

@bakkot
Copy link
Contributor

bakkot commented Dec 2, 2024

As a data point, rust's Option type (not Map) has get_or_insert (insert value if not present) and get_or_insert_with (insert value computed from callback if not present).

I like getOrInsertComputed much better than getOrInsertWith ("with" what?), so I'm not suggesting it as a name, just mentioning it as prior art. Rust generally uses _with for the callback-taking form of methods like this (e.g. Entry's or_insert_with).

@dminor
Copy link
Collaborator Author

dminor commented Dec 2, 2024

rbuckton suggested getOrAdd/getOrCreate for these

@bakkot
Copy link
Contributor

bakkot commented Dec 2, 2024

"create" sounds like it's going to new the callback to me.

@gibson042
Copy link

Within at least the Agoric codebase, there is precedent for a provide accepting a key and a callback to return the initial computed value (e.g., map.provide(key, makeInitialValue)).

@PowerKiKi
Copy link

Since existing methods are get() and set(), why not combine both, to stay in familiar territory, with getOrSet() and getOrSetComputed() ?

@dminor dminor mentioned this issue Dec 3, 2024
17 tasks
@belgattitude
Copy link

@PowerKiKi . I second getOrSet for the same reasons to mention. I feel it more consistent and a tiny shorter too.

@0f-0b
Copy link

0f-0b commented Dec 31, 2024

C++ simply calls this operation insert. The return value consists of an iterator (generalized pointer) to the inserted or existing element, and a bool indicating whether the insertion took place.

@jahudka
Copy link

jahudka commented Jan 1, 2025

I might make a complete tool out of myself here, but why not just make it an overload for .get()?

export class Map<K, V> {
  get(key: K): V | undefined;
  get(key: K, fallback: V | (() => V)): V;
}
  • It's backwards-compatible
  • It doesn't bloat the API with new methods
  • It's a pattern I've definitely seen used elsewhere (can't remember now, maybe Symfony cache?)

@bakkot
Copy link
Contributor

bakkot commented Jan 2, 2025

There's a few problems with that suggestion, the most significant of which is that a method named get should not be mutating the map.

@jahudka
Copy link

jahudka commented Jan 3, 2025

@bakkot I think the use case which this proposal caters to is an almost perfect analogy for lazy accessors - and it's typically the get accessor that's used to lazily compute a value on demand and store it for later use. So a get method mutating the object doesn't feel too weird to me.

Of course, that's still addressing only the one problem you mention explicitly; I don't know which other problems there might be. And I've said my piece about it and I promise I'm not gonna argue my point further - I'll just end by noting that I feel that in general, reusing existing vocabulary of an API where possible feels like it's more accessible / less confusing to people who are unfamiliar with it, especially if English isn't their first language - so personally I'd be much more in favour of e.g. getOrSet than something like insert (which, next to set, sounds to me like insert at a specific position, ie. a different operation), getOrAdd (which leads me to expect there would be an add), or especially upsert (which I think few people who never worked with SQL would understand at a glance), or even worse, emplace (which, to be fair, I didn't even know was actually a word until I googled it just now).

I love the simplicity of get and its symmetry with the other main three-letter methods of Map and Set, but if it must be something else, getOrSet feels like the best option.

@dminor
Copy link
Collaborator Author

dminor commented Feb 3, 2025

I'm thinking that getOrSet and getOrSetComputed are the best options, because they're shorter and line up with the existing method being called set and not insert.

Does anyone object to these names strongly enough that I should bring this issue for discussion at an upcoming TC39 plenary? I'd prefer to not have to use committee time to resolve naming the methods if I can avoid it.

@acutmore
Copy link

acutmore commented Feb 3, 2025

My personal preference is still getOrInsert, similar to #60 (comment), set returns the receiver so I think introducing a new verb is justified.
I can even imagine a (albeit unlikely) future where we added an insert method to Map which did return the value.

If a majority are happy with getOrSet that also sounds Ok.

@rkirsling
Copy link
Member

I guess I don't feel too worried about the return type of set just because getOrSet would be fundamentally unusable if it returned the receiver part of the time. Basically it is first and foremost a get operation.

@dminor
Copy link
Collaborator Author

dminor commented Feb 4, 2025

My personal preference is still getOrInsert, similar to #60 (comment), set returns the receiver so I think introducing a new verb is justified. I can even imagine a (albeit unlikely) future where we added an insert method to Map which did return the value.

That is a good point, I'm also ok with getOrInsert, let's see if there's a majority opinion here. Thanks for the feedback.

@bakkot
Copy link
Contributor

bakkot commented Feb 4, 2025

Let's do a quick check.

Everyone please 👍 this comment if you are happy with getOrInsert.

@bakkot
Copy link
Contributor

bakkot commented Feb 4, 2025

Everyone please 👍 this comment if you are happy with getOrSet.

(You can also 👍 both, or neither.)

@dminor
Copy link
Collaborator Author

dminor commented Feb 5, 2025

Please vote (or raise objections to one of the options) by end of day Friday.

Next week, I'll do one of: close this issue, create a pull request to change the names, or add an item to the next TC39 agenda, depending upon the results here.

@bthall16
Copy link

bthall16 commented Feb 5, 2025

getOrInsert seems more clear to me personally. Inserting into a Map sounds like something you'd do once (more specifically, when the key isn't present), while setting a value can happen at any point.

Consequently, the name getOrSet makes me think "well, which one are you going to do?" while getOrInsert makes me think "get the value if you can or insert if you can't."

getOrSet does nicely align with with existing Map terminology, but that's specifically why I think it's not as good a name as getOrInsert: the set method could be inserting or updating the entry, so getOrSet doesn't seem to convey the point that the method will only ever insert an entry, never update.

@acutmore
Copy link

acutmore commented Feb 5, 2025

@bthall16 that's a really great explanation. I could feel in my gut that I preferred "insert" and you've put it into words much better than I could.

@dminor
Copy link
Collaborator Author

dminor commented Feb 11, 2025

I think @bthall16 made a very good case for keeping getOrInsert and getOrInsertComputed, and it had a slight edge in votes as well, so it makes sense to keep the existing names.

@dminor dminor closed this as completed Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests