Decide API for setproperties and constructorof

[jw3126]

@tkf @rafaqz some API questions:

• Shall we change the signature of setproperties? Currently it is setproperties(obj, patch). But maybe setproperties(obj; kw...) is more convenient?
• Shall we export constructor_of? Shall we rename it into e.g. positional_constructor?

[tkf]

But maybe setproperties(obj; kw...) is more convenient?

I'd say setproperties(obj, patch) is better. For example, we may want to support other signatures like setproperties(obj, (Val(2) => val2, (Val(5) => val5))). Also, keyword arguments produces extra functions and it could be bad if you are close to the inline threshold boundary.

In general, I think it'd be better for ConstructionBase to aim for compositionality rather than end-user convenience.

Shall we export constructor_of?

I suppose ConstructionBase's API surface would be small and not overlap with Base? So, I think exporting all public API is a good baseline policy.

Shall we rename it into e.g. positional_constructor?

Hmm... That's kind of make sense. But maybe we are not going to have keyword_constructor? Also, since positional constructor is the default constructor you get for free from struct MyStruct ... end, I think it makes sense to call it just "constructor."

Another naming bikeshed is whether or not we should use constructorof to be compatible with typeof, sizeof etc. Or maybe even just constructor. I'm somewhat against constructor since it's handy to name the return value constructor:

constructor = constructor_of(T)

As for constructor_of vs constructorof, Julia documentation says:

• functions are lowercase (maximum, convert) and, when readable, with multiple
  words squashed together (isequal, haskey). When necessary, use underscores
  as word separators. Underscores are also used to indicate a combination of concepts (remotecall_fetch
  as a more efficient implementation of fetch(remotecall(...))) or as modifiers.

--- https://docs.julialang.org/en/latest/manual/style-guide/#Use-naming-conventions-consistent-with-Julia-base/-1

...so is constructorof readable? Personally, I find constructorof hard to parse.

[tkf]

Ref: Underscore audit JuliaLang/julia#20402 (comment)

For example, it looks like include_string survived the API review. (Although there was not much discussion?)

[rafaqz]

I like constructorof for the fact that it matches typeof and has a very similar role. I agree it is harder to parse, and in general Julias no-underscore approach doesn't align with research on variable name readability, but it is what it is.

We should export it for sure.

[tkf]

OK. Considering there are many SOMETHINGof functions, maybe the incentive of being consistent is much stronger than the incentive of being readable.

[jw3126]

About constructor(_)(of) naming I am usally much more liberal with underscores then Base. But I thing there are reasonable arguments for each variant. So lets go with constructorof.

In general, I think it'd be better for ConstructionBase to aim for compositionality rather than end-user convenience.

I think that is an excellent design guideline. So lets go with setproperties(obj, patch).
I suspect however that downsteam packages want to expose the ;kw... variant. So maybe also introduce reconstruct(obj; kw...) that potential downstream packages like DiffEq.jl and Parameters.jl can rexport instead of implementing their own?

It was brought up to move @settable to this package in #2. I agree that if @settable has to live somewhere, here is a good place. But it is ugly and introduces a MacroTools dependency. Maybe it is possible to get rid of @settable all together?
Personally I only use it to patch QuickTypes definitions. But it looks like that will not be needed in future: jw3126/Setfield.jl#68 (comment)
What do you think?

[tkf]

I suspect however that downsteam packages want to expose the ;kw... variant.

I think it's a good point that downstreams might want keyword arguments variant. So I guess defining it here makes sense, provided that it is documented as call-only API; i.e., type authors should overload setproperties(obj::MyType, patch), not setproperties(obj::MyType; kwargs...). Also, if we are going to implement keyword arguments-version, I think it makes sense to call it setproperties as those two APIs do the same thing with different call syntax.

It was brought up to move @settable to this package in #2.

Personally I only use it to patch QuickTypes definitions. But it looks like that will not be needed in future: jw3126/Setfield.jl#68 (comment)

Good point. Actually, I started think that bringing @settable here is not a good idea because...

• It doesn't depend on anything and it is not a dependency of anything (OK, maybe testing).

• It's just a convenient way of defining constructorof-compatible constructor. It can live in a separate package which makes more sense now that we have https://github.com/JuliaObjects

• There is a hope that we can get rid of it once struct-defining packages (e.g., QuickTypes.jl) start using ConstructionBase.jl. If that happens, we don't want to keep maintaining @settable.

[jw3126]

Ok should these two

setproperties(obj::MyType, patch)
setproperties(obj::MyType; kwargs...)

have the same name or maybe call the one patchproperties(obj, patch)?

[tkf]

setproperties sounds good to me. patchproperties sounds like it supports other diff thing like removing properties.

[jw3126]

So lets leave @settable in Setfield and hope that it can be removed altogether in future.

[jw3126]

Summary so far:

• Spelling is constructorof
• We have setproperties(obj; kw...) and setproperties(obj, patch). Only the latter is allowed for overloading.

Does that sound good? Any other API issues about these two functions?

[tkf]

Thanks a lot for initiating this repo! LGTM other than one trivial tweak.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@08c63a0). Click here to learn what that means.
The diff coverage is 92.85%.

@@            Coverage Diff            @@
##             master       #1   +/-   ##
=========================================
  Coverage          ?   92.85%           
=========================================
  Files             ?        1           
  Lines             ?       14           
  Branches          ?        0           
=========================================
  Hits              ?       13           
  Misses            ?        1           
  Partials          ?        0

Impacted Files	Coverage Δ
src/ConstructionBase.jl	92.85% <92.85%> (ø)

---

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 08c63a0...ecaf7e0. Read the comment docs.

[jw3126]

Thanks for setting up travis + codecov!