Remove mutation support #34
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
========================================
+ Coverage 96.09% 97.6% +1.5%
========================================
Files 4 3 -1
Lines 205 125 -80
========================================
- Hits 197 122 -75
+ Misses 8 3 -5
Continue to review full report at Codecov.
|
| @@ -4,7 +4,7 @@ os: | |||
| - linux | |||
| - osx | |||
| julia: | |||
| - 0.6 | |||
| - 0.7 | |||
|
Do you have some release plan? How about doing it after merging this? FYI using this branch with my package passed its tests https://travis-ci.org/tkf/Bifurcations.jl/builds/414973794 (although my Setfield usage is very basic). |
src/lens.jl
Outdated
|
|
||
| const NNamedTupleLens{N,s} = NamedTuple{s, T} where {T <: NTuple{N, Lens}} | ||
| struct MultiPropertyLens{L <: NNamedTupleLens} <: Lens | ||
| lenses::L |
There was a problem hiding this comment.
Why not use just a tuple of lenses, rather than a named tuple? With a named tuple, you can only support properties, right? (Though maybe that was the intention as you named it MultiPropertyLens?) I mean, I wonder if it makes sense to support "vectorizing" lens of mixed kinds, like this:
obj0 = (a=1, b=2, c=3)
l = @lens( # this is a made-up syntax
_.a, # get/set by property
_[3], # get/set by index
)
obj1 = set(l, obj0, (100, 300))
@assert obj1 == (a=100, b=2, c=300)There was a problem hiding this comment.
Motivation was that I had lots of code, which looks like
obj = @set obj.a = a1
obj = @set obj.b = b1
obj = @set obj.c = c1And the compiler was often unable to make this as fast as some handwritten
obj = Obj(a1,b1,c1, obj.other_fields...)I like your approach, but I think it is harder for the compiler. I need to play around with it. Anyway this is a separate concern from removing mutation policy. It was a mistake to add it to this branch and I will remove it.
There was a problem hiding this comment.
Yeah, I agree that it's nice to release a simple version now and do some design iteration. For the multi-lens, let's continue discussion and design iteration in #23 (comment)
test/test_quicktypes.jl
Outdated
| @@ -69,7 +69,7 @@ end | |||
| @testset "Pack" begin | |||
| x = Pack([Cat(:Tama, 1), Cat(:Pochi, 2)]) | |||
|
|
|||
| @set! x.animals[2].nlegs = 5 | |||
| x = @set x.animals[2].nlegs = 5 | |||
There was a problem hiding this comment.
I think this part is not tested since I commented test_quicktypes.jl out in #31 (comment). I suggest to pull #36 first or cherry-pick it.
There was a problem hiding this comment.
Ah, so the mutability tests fail there with Julia 1.0. I guess this PR should go first.
There was a problem hiding this comment.
So #37 is failing as expected.
|
Tagging a new release after removing mutation support sounds fine to me! |
* Test against Julia 1.0 * Use the default test script * Fix after_success for Julia 1.0 * Enable tests with QuickTypes * Use Tuple instead of Vector in test_quicktypes.jl
This reverts commit d8a8730.
|
@tkf does this look good to you, anything missing? |
|
Yup, #39 was all what I wanted to add. It's also great that MacroTools stuff goes upstream (congrats!). I think it's ready to go! |
|
I tagged a new release! Thanks for #39! |
|
I didn't follow this package very closely; why was |
|
My motivation for removing it was, that I did not use it, but it added baggage to each custom lens definition. Do you need it? |
|
There is some discussion here #32 |
|
I haven't tried using this package yet, but I'd like to use it to modify our default configuration. It would be cleaner to write @set! begin
config.thresh = 0.1
config.N = 20
endor @set! config begin
thresh = 0.1
N = 20
endor I can write these macros myself on top of I don't know if |
|
Ah okay |
No description provided.