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

@eval code generation is kind of confusing! #112

Open
rafaqz opened this issue Sep 25, 2018 · 2 comments
Open

@eval code generation is kind of confusing! #112

rafaqz opened this issue Sep 25, 2018 · 2 comments

Comments

@rafaqz
Copy link
Contributor

rafaqz commented Sep 25, 2018

I was trying to track some broken styling in inputs.jl, but the eval blocks make it a little hard to track what is happening! I'm wondering if it is worth having them at all, in terms of the obfuscation vs DRYness ratio they deliver.

You could get a lot off the way by just extracting a few shared functions, say in slider.jl:

slider(WT::WidgetTheme, vals::AbstractArray, formatted_vals = format.(vec(vals)); value = medianelement(vals), kwargs...) =
    build_slider(slider, WT, vals, formatted_vals, value)

rangeslider(WT::WidgetTheme, vals::AbstractArray, formatted_vals = format.(vec(vals)); value = medianelement(vals), kwargs...) =
    build_slider(rangeslider, WT, vals, formatted_vals, value)

function build_slider(f, WT, vals, formatted_vals, value)
    T = Observables._val(value) isa Vector ? Vector{eltype(vals)} : eltype(vals)
    value isa AbstractObservable || (value = Observable{T}(value))
    vals = vec(vals)
    indices = axes(vals)[1]
    f = x -> _map(t -> searchsortedfirst(vals, t), x)
    g = x -> vals[Int.(x)]
    ObservablePair(value, f = f, g = g).second

    value = build_value(vals, value)
    index = build_index(vals)

    wdg = Widget(f(WT, indices, formatted_vals; value = index, kwargs...), output = value)
    wdg["value"] = value
    wdg
end

Anyway, that is just my 2 cents! And mostly because I imagine I'll be reading this code a lot more, because its so useful :)

@piever
Copy link
Collaborator

piever commented Oct 1, 2018

The slider stuff was actually very tricky to get right because it needs to work with arbitrary AbstractVector of potentially "unitful" quantity but at the same time I wanted to keep simple things (slider of consecutive integers) simple (meaning no extra julia - javascript communication) and accomodate both interfaces (native html and the nouislider API).

I don't mind so much the@eval code generation, but it's probably important to comment the code to clarify what methods are called and how: for slider this is actually quite a tricky pipeline, as it's first getting the indices of the array and the values formatted by Julia, then calling the method for indices and formatted values, which in turns calls the method for AbstractArray{<:Integer} and adds some javascript to show the formatted values correctly.

@rafaqz
Copy link
Contributor Author

rafaqz commented Oct 1, 2018

I was trying to write it as a pure demonstration refactor with no changes to functionality, but I didn't test it so maybe it is actually different.

I'm also fine with the @eval but it does take a lot longer to understand.

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

2 participants