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

Define functional setters using Setfield #191

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Conversation

tkf
Copy link
Contributor

@tkf tkf commented Oct 8, 2019

With this PR, you can do:

julia> using VegaLite

julia> using Setfield  # to import `@set`

julia> plt = NamedTuple{(:x, :y)}.(tuple.(randn(1000), randn(1000))) |>
       @vlplot(
           :point,
           x=:x,
           y=:y,
       )

julia> plt2 = @set plt.mark = :line

This is built on top of #190 and will close #168.

@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

Merging #191 into master will decrease coverage by 0.24%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   84.87%   84.63%   -0.25%     
==========================================
  Files          12       12              
  Lines         410      423      +13     
==========================================
+ Hits          348      358      +10     
- Misses         62       65       +3
Impacted Files Coverage Δ
src/VegaLite.jl 100% <ø> (ø) ⬆️
src/spec_utils.jl 84% <88.88%> (-7.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37614d1...74c3d0a. Read the comment docs.

@davidanthoff
Copy link
Member

Cool, so this now is purely functional, with no mutation, right?

@tkf
Copy link
Contributor Author

tkf commented Oct 9, 2019

Yep, it's purely functional. Internally, Setfield uses something called lens which is invented by a Haskeller: https://github.com/ekmett/lens

@davidanthoff
Copy link
Member

Alright, yes, then lets do this. Do you have to rebase now that #190 is merged, or do we merge this right away?

@tkf tkf closed this Oct 9, 2019
@tkf tkf reopened this Oct 9, 2019
@tkf
Copy link
Contributor Author

tkf commented Oct 9, 2019

It looks like it has no conflict so it can be merged as-is? But let me know if you want me to rebase.

(I just restarted the CI for now.)

@davidanthoff
Copy link
Member

The diff here on the PR looks weird: it looks like it will add ObjectProxy, but that is already on master, right? Slightly confused...

@tkf
Copy link
Contributor Author

tkf commented Oct 10, 2019

Oh, I wasn't looking at the diff. I rebased the PR.

Comment on lines +39 to +43
function Setfield.set(spec::ObjectLike, ::PropertyLens{name}, value) where name
params = copy(getparams(spec))
params[keytype(params)(name)] = _maybeparams(value)
@set getparams(spec) = params
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be written as @set getparams(spec)[keytype(params)(name)] = value if Base had setindex for Dict...: JuliaLang/julia#33495

@davidanthoff davidanthoff merged commit 5b5d789 into queryverse:master Oct 10, 2019
@davidanthoff
Copy link
Member

Thanks, I really like this now! If you have time, maybe add some docs as another PR?

@tkf
Copy link
Contributor Author

tkf commented Oct 10, 2019

Will do. Nice thing about Setfield is that it works with other constructs. Things like

obj = (
    plt1 = @vlplot(...),
    plots = (
        @vlplot(...),
        @vlplot(...),
        @vlplot(...),
    ),
)

@set obj.plt1.mark = :point
@set obj.plots[2].mark = :line
@set last(obj.plots).mark = :line

should work now.

@davidanthoff
Copy link
Member

Ok, that is pretty amazing!

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.

API to manipulate Vega Lite properties directly?
3 participants