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

Make ScopedValues.get a method of Base.get #54553

Open
LilithHafner opened this issue May 22, 2024 · 9 comments
Open

Make ScopedValues.get a method of Base.get #54553

LilithHafner opened this issue May 22, 2024 · 9 comments
Labels
design Design of APIs or of the language itself

Comments

@LilithHafner
Copy link
Member

LilithHafner commented May 22, 2024

ScopedValues.get(::ScopedValues.ScopedValue) performs a very similar role as Base.get(collection, index, default) and I think they should have the same API and name, i.e. rename ScopedValues.get to Base.get and make its API conform to the Base get API.

Right now, ScopedValues.get is it's own generic function, distinct from Base.getindex and from Base.get.

ScopedValues was introduced in 1.11, so this needs to be done before the 1.11 release if we want to do it at all.

Reading #50958, I don't see any explicit discussion about this name, but perhaps I'm missing something.

cc @vchuravy

@LilithHafner LilithHafner added design Design of APIs or of the language itself triage This should be discussed on a triage call labels May 22, 2024
@LilithHafner LilithHafner added this to the 1.11 milestone May 22, 2024
@vchuravy
Copy link
Member

I am confused? get and getindex both exist?

@LilithHafner
Copy link
Member Author

LilithHafner commented May 22, 2024

Ah, silly me, getindex(::ScopedValue) is already defined, yes. Did we consider making ScopedValues.get be a method of Base.get and follow the Base.get API, though without the key argument?

@DilumAluthge
Copy link
Member

Maybe this line:

function get(val::ScopedValue{T}) where {T}

Should be changed to this instead?

- function get(val::ScopedValue{T}) where {T}
+ function Base.get(val::ScopedValue{T}) where {T}

@DilumAluthge
Copy link
Member

Ah Lilith beat me to it.

@vchuravy
Copy link
Member

ScopedValues.get doesn't match the semantics of get so I intentionally didn't use Base.get

@LilithHafner LilithHafner changed the title Rename ScopedValues.get to Base.getindex Make ScopedValues.get a method of Base.get May 23, 2024
@LilithHafner LilithHafner removed this from the 1.11 milestone May 23, 2024
@LilithHafner
Copy link
Member Author

We can do this after 1.11 in a nonbreaking way.


From triage: it's complicated, let's talk about it later.


In the future we could define this and merge the functions

mutable struct Secret end
const SECRET = Secret()
function Base.get(x, index) 
	res = get(x, index, SECRET)
    res === SECRET ? nothing : Some(res)
end

@KristofferC
Copy link
Member

Base.get takes three arguments so it's unclear what the meaning of that two arg get is.

@JeffBezanson
Copy link
Member

I think if Base.get had 1- or 2-argument methods they would work like this, and also if ScopedValue.get had a 3-argument method it would work like Base.get, so they seem compatible to me.

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Jan 2, 2025
@vtjnash
Copy link
Member

vtjnash commented Jan 22, 2025

#51362 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design of APIs or of the language itself
Projects
None yet
Development

No branches or pull requests

6 participants