search

jw3126 / Setfield.jl

Update deeply nested immutable structs.
Other
167 stars 17 forks source link

better printing for lens types #97

Closed simeonschaub closed 5 years ago

simeonschaub commented 5 years ago

Addresses #85. Also uses this better printing for types PropertyLens, ConstIndexLens and ComposedLens.

jw3126 commented 5 years ago

Thanks for this and #98. The main motivation for changing show of FunctionLens was, that the type parameters of FunctionLens are not part of the stable API. See https://jw3126.github.io/Setfield.jl/latest/#Setfield.FunctionLens and https://github.com/jw3126/Setfield.jl/pull/77#issuecomment-530551511 . So it makes sense to print this type like we want people to use it in overloading signatures. That does not apply to the other lenses. So I am unsure if I like "hiding" their type. What do you think @tkf?

tkf commented 5 years ago

@simeonschaub Thanks for giving this a shot!

@jw3126 Yeah, doing only for FunctionLens makes sense. Overloading Type{...} still is not really the safest thing we can do. I know Zygote.jl does it but it also was bitten by it https://github.com/FluxML/Zygote.jl/issues/134

Also, IIRC show for lens values do not differentiate a ∘ (b ∘ c) and (a ∘ b) ∘ c. Doing this at type level would be confusing.

simeonschaub commented 5 years ago

Ok, thanks for the feedback! Closing this in favor of #99