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

RFC: Add @get! #1764

Closed
wants to merge 1 commit into from
Closed

RFC: Add @get! #1764

wants to merge 1 commit into from

Conversation

toivoh
Copy link
Contributor

@toivoh toivoh commented Dec 15, 2012

As per this discussion.
Already, get(d::Associative, key, default) returns the value at key if one exists, or default otherwise.

This patch adds get!, @get, and @get!:

  • get! and @get! assign default to d[key] if it's not present.
    They return d[key] (i.e. the converted value).
  • @get and @get! evaluate default only if key is missing.

Thoughts? I think that at least @get! would provide a useful addition to Base.

Edit: I removed everything but @get! (and made it avoid the extra lookup when applied to a Dict)

@JeffBezanson
Copy link
Sponsor Member

I like get! a lot. One disadvantage of @get though is that it does the lookup twice if the key exists, unlike get!. It also feels like get, get!, @get, and @get! is just too many gets :)

@ViralBShah
Copy link
Member

This pull request should also include an update to the documentation, so that one knows which get to use, assuming we need all of these.

@timholy
Copy link
Sponsor Member

timholy commented Dec 16, 2012

In get!, why doesn't has(d, k) yield one lookup and d[k] a second one?

Toivo, you can use ht_keyindex to make sure it only happens once.

@get! works like get, but evaluates the default value only if the key is not
present; it then assigns the default value to the key and returns the
(converted) value
@toivoh
Copy link
Contributor Author

toivoh commented Dec 16, 2012

All good points.
@JeffBezanson: I also feel that get! is the major addition. Though when I have used it, I almost always wanted the @get! variety, since I would construct a new object to serve as the default. So I've updated the pull request to provide only @get!.
@ViralBShah: It should definitely be added to the docs. I'll do that once we have decided what functionality goes into the patch.
@timholy: I think you saw my fallback definition of get!. I provided one specific to Dict further down, that used ht_keyindex.

About doing the lookup twice: It didn't feel sane to provide a @get! macro that would work on Dict:s only.
I wasn't sure how to do it in a general way without breaking the abstraction of Dict (ht_keyindex feels like an implementation detail of Dict to me.)

Anyways, I have now eliminated the second lookup for @get! in the pull request by introducing a semi-inofficial interface for Associative subtypes to expose the equivalent of ht_keyindex (with sensible defaults). Please tell me how you like it.

Once we have specialization on function arguments, I think a cleaner implementation would be with a method

_get!(d::Associative, k, default::Function)

where default is a thunk that gets evaluated only if the key is missing. Then @get! could be implemented on top of this instead.

@StefanKarpinski StefanKarpinski mentioned this pull request Jan 9, 2013
9 tasks
@diegozea
Copy link
Contributor

I found a mutation version of get useful and intuitive when you want to fill a Dict. I hope @get! or other mutating version wiil be add into Base :)

@StefanKarpinski
Copy link
Sponsor Member

How about having some syntax for get!(dict,key,default)? Given how much programming in languages like Python, Perl and Ruby involves doing stuff with dicts/hash tables, I would consider it worthwhile to consider adding this.

@toivoh
Copy link
Contributor Author

toivoh commented Jan 10, 2013

My original pull request included get!, @get, and @get!, but there was a feeling that that was too many gets, so I cut it down to what I thought was essential. My experience has been that usually when I want to use a mutating get, it will be wasteful to construct the default at each invocation. How about adding get! and @get!?

@timholy
Copy link
Sponsor Member

timholy commented Jul 24, 2013

Bump. Personally I prefer this to the operator idea.

@StefanKarpinski
Copy link
Sponsor Member

Unsurprisingly, I prefer the operator proposal – just the original bit. Using macros for something as basic as this seems really ugly to me.

@thomasmcoffee
Copy link

Please keep @get (however it's implemented) ... of the four variants, I find this is the one I want most often.

It seems much less confusing to the user to have all the forms than to have one of the four arbitrarily missing. "Too many gets" is much less annoying than "all the gets except the one you want."

The manual could make it really obvious with a simple table:

|                               | no update | update |
|-------------------------------+-----------+--------|
| don't evaluate unused default | @get      | @get!  |
|       evaluate unused default |  get      |  get!  |

@StefanKarpinski
Copy link
Sponsor Member

Since we added the higher-order method for get, I think we can close this. I still favor adding some syntax for default assignment – i.e. d[k] ?= v – I don't really think that we need syntax for the non-assigning version, which seems far less common in my experience.

@toivoh
Copy link
Contributor Author

toivoh commented Apr 8, 2014

Yes, we should keep track of the operator somewhere.

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

Successfully merging this pull request may close these issues.

None yet

7 participants