Closed jw3126 closed 6 years ago
Merging #17 into master will decrease coverage by
0.48%
. The diff coverage is86.51%
.
@@ Coverage Diff @@
## master #17 +/- ##
==========================================
- Coverage 87.61% 87.12% -0.49%
==========================================
Files 3 5 +2
Lines 113 233 +120
==========================================
+ Hits 99 203 +104
- Misses 14 30 +16
Impacted Files | Coverage Δ | |
---|---|---|
src/Setfield.jl | 50% <ø> (-50%) |
:arrow_down: |
src/lens.jl | 83.33% <50%> (-1.58%) |
:arrow_down: |
src/macrotools.jl | 81.81% <81.81%> (ø) |
|
src/settable.jl | 93.02% <93.02%> (ø) |
|
src/sugar.jl | 90% <0%> (+0.34%) |
:arrow_up: |
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 1203f28...b4718f0. Read the comment docs.
Cool! I'm on trip right now. So it'll take a few more days to try it out.
I suggest to generate the body of positional-only constructor from user-defined inner constructor (implementation in #18).
This helps for enforcing some invariants, e.g.:
julia> struct OrderedPair x::Real y::Real OrderedPair(x,y) = x > y ? error("out of order") : new(x,y) end
--- https://docs.julialang.org/en/latest/manual/constructors/#Inner-Constructor-Methods-1
This version also adds the body of the first constructor to the positional constructor. So invariants in the body of the first constructor are enforced.
Oooh, yes, sorry, you are right. I thought I had a good reason when started writing a patch...
So the main difference between #17 and #18 is that I defined the positional-only inner constructor solely based on user-defined inner constructor in #18. On the other hand, your default_constructor_dict
in #17 generates function signature (args, kwargs, params and whereparams) from field and struct type parameters. I think this way of generating positional-only constructor is limiting, since:
(1) User may write some "type restriction" in where
part of the constructor. We will miss it if we use the ones from struct where
. On the other hand, the where
in struct would be imposed when invoking new
so we don't need to worry about it.
(2) User may use different type parameter names in constructor than in struct type parameters. It is awful coding habit, but we need to have a way to detect and abort for such case than runtime cryptic UndefVarError
errors, if we go with #17's default_constructor_dict
. #18 would allow such code so it is in a way more robust and we don't need to write extra code to rule out such bad code.
(3) User may add a custom promotion rule inside the struct. It will be incompatible with the function signature derived from struct type parameters. Example:
struct RangeWithInitial{X}
xmin_xmax::Tuple{X,X}
x0::X
function RangeWithInitial(xmin_xmax::Tuple, x0)
xmin, xmax, x0 = promote(xmin_xmax..., x0)
return new{typeof(x0)}((xmin, xmax), x0)
end
end
constructor_of
definitionI think it is highly likely that people write constructor without "{}
"
@settable struct ABC{A,B,C}
a::A
b::B
c::C
function ABC(a::A; b::B = 1, c::C = 2) where {A, B, C}
... some code ...
return new{A, B, C}(a, b, c)
end
end
than with "{}
":
@settable struct ABC{A,B,C}
...
function ABC{A,B,C}(a::A; b::B = 1, c::C = 2) where {A, B, C}
# ^^^^^^^
...
end
end
In current status of Setfield.jl, without-{}
constructor needs custom constructor_of
:
constructor_of(::Type{<: ABC}) = ABC
to make setproperty
work. So I suggest to modify default constructor_of
to:
constructor_of(T::Type) = getfield(T.name.module, Symbol(T.name.name))
constructor_of(T::UnionAll) = constructor_of(T.body)
to get the type-parameter -less version of the type constructor. The default setproperty
function then works with type-parameter -less constructor out-of-the-box. Note that it works with default constructor since Julia generates type-parameter -less constructor as well.
The reason why I like this approach is that it's easy/trivial to get “differentiable lens” if constructor_of
returns the type-parameter -less constructor.
... as well as other situations in which changing type parameters makes sense: https://discourse.julialang.org/t/ann-setfield-jl-yet-another-package-for-working-with-immutables/9267/11?u=tkf
So, type-parameter -less constructor works, without breaking @inferred
tests: #19
@tkf should we merge this into master or does it require further changes? Does the @settable
macro here suffice to migrate your uses of Reconstructables.jl
to Setfield.jl
?
Yes, I think so, in principle. I haven't started using Reconstructables.jl yet, so I don't have a practical codebase to try it out. (As actually I already started using Setfield.jl, since I realized lens was great!) But I think our test covers important cases, so I think this PR is good to go.
@settable
is a no-op?We can worry about this later, but maybe it's good to throw in @settable
when it is no-op? I mean the test cases like:
@settable struct NoConstructor{A,B}
b::B
end
@settable struct ExplicitConstructor{A,B}
b::B
ExplicitConstructor{A,B}(b::B) where {A,B} = new{A,B}(b)
end
I think the users who would put @settable
there may be expecting something else than no-op to happen; otherwise they won't put @settable
there. There are three possibilities:
No-op (i.e., current @settable
).
@settable
. It is cumbersome to remove @settable
.Warn.
@settable
when debugging.Throw.
If we want to be extremely careful here and try to be forward-compatible, I'd go with the option 3 (throw) since that's the only option that is forward-compatible in case we want to switch to option 1 or 2.
The only "danger" with the no-op I see, is that new users could think Setfield
requires to annotate each and every singe type definition. As you say error is cumbersome for debugging. I would be fine with a warning or just an info on the other hand. If we want to change the behavior in future, we can still add a depwarn in the no-op case.
I don't really have an opinion, should we add a warning, you decide :)
If we support QuickTypes and alike as in #21, we need to make @settable
no-op for many kinds of statements. So let it be no-op, for consistency. (What a great excuse for being lazy :smile:)
Ok lets merge this!
13
@tkf can you play around with this branch and see if it fits your needs?