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 forwarding of methods like getindex, getproperty etc. #94

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hhaensel
Copy link

@hhaensel hhaensel commented Jul 5, 2022

This is a PR to the discussion on silent assignment #77.

@SimonDanisch
I'm not sure whether I placed all @nospecialize in the right places as I'm not an expert on inference (trying to get into it ...)
E.g. line 103, I did not place @nospecialize to make the lookup of :field in fieldnames(T) faster. Is that notion correct? Moreover, compilation of that code should be fast and there won't be very many types around, so not too big a waste in terms of dispatch tables?

@hhaensel
Copy link
Author

hhaensel commented Jul 5, 2022

What more functions could we add to the list of forwarded functions.
Currently, we have push!, pushfirst!, pop!, popfirst!
What about empty!, reverse!, drop! etc... ?

@hhaensel
Copy link
Author

hhaensel commented Sep 7, 2022

@SimonDanisch
May I ask what's your opinion here, also with respect of bringing this feature into v0.5?
I saw that v0.5 is registered but not tagged as a release. - So should I not yet use v0.5 in current package releases?

@SimonDanisch
Copy link
Member

Sorry, totally dropped the ball on this...
One of the reasons is, that I had quite a different implementation in mind, more like what I did here: https://github.com/SimonDanisch/ShaderAbstractions.jl/blob/master/src/types.jl#L12-L65.

@hhaensel I'm not sure if we were miscommunicating, or what happened here but I took from the comments in #77:

Your idea is quite the same as mine.
Yeah, that's what we did

That the implementation would be closer to the one in ShaderAbstractions :D What the PR implements here seems pretty different, though (no clean separation between Observable and ArrayObservable and no way to subscribe to e.g. setindex! specifically).

I don't think the default observables should forward array behavior, especially not for something like Observable{Int}.
I'd be fine with obs[!] = new_val for obs.val = new_val, if @jkrumbiegel @timholy agree.
Although the bang, semantically, kind of indicates the opposite. That's why I was always quite happy with obs.val = new_val because it pretty clearly shows the intention (setting the field directly without triggering anything 🤷).

@hhaensel
Copy link
Author

I, indeed, misunderstood what your implementation was supposed to do, my bad.

I don't think the default observables should forward array behavior, especially not for something like Observable{Int}.

Actually, your code from line 59 inspired me to include forwarding of array operations. I certainly don't cry to cancel that, because it's difficult to tell where to stop.

However, forwarding of setindex!() and setproperty!() is something I find very convenient, because it very naturally combines element update + notification. And it does not break code, because that code would simply have generated an error before.

I'd be fine with obs[!] = new_val for obs.val = new_val, if @jkrumbiegel @timholy agree.
Although the bang, semantically, kind of indicates the opposite. That's why I was always quite happy with obs.val = new_val because it pretty clearly shows the intention (setting the field directly without triggering anything 🤷).

I think the point of defining a [!] syntax is that it would provide an official API and any derived type would also be encouraged to support that notation. That would make it unnecessary to chain myobs.o.val = x, instead it would simply be myobs[!] = x.

One last idea, if forwarding is not an option for the universal AbstractObservable it could be an option to offer an extra type, e.g. AbstractDeepObservable (I don't have a better name in the moment) which does support the forwarding.

@jkrumbiegel
Copy link
Contributor

Shouldn't we use a normal function, for example mutate!() or so instead of that special [!] syntax? It doesn't seem worth the convenience to me to use such a nonstandard idiom.

@hhaensel
Copy link
Author

hhaensel commented Nov 13, 2022

I understand your point. But in that case I would prefer the standard syntax obs.val = x, just as @SimonDanisch mentioned.

I could, however, think of a more general function mutate!(f::Function, obs::Observable), that takes a filterfunction for priorities, as now there are priorities attached to the listeners. @jkrumbiegel 's version would then be a second method without filterfunction.

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.

3 participants