use properties, not fields

[aplavin]

fix #59

[jw3126]

@rafaqz what do you think? Should we just drop julia < v1.7 support? The alternative would be to base getproperties on fieldnames/propertynames based on the julia version. Which I think is a bad idea.

[jw3126]

If this works for getproperties, it might also be possible to remove the @generated here https://github.com/JuliaObjects/ConstructionBase.jl/blob/master/src/ConstructionBase.jl#L37 ?

[aplavin]

Should we just drop julia < v1.7 support?

Well, the same function works, but slower - not for the tightest loops.

See the new commit: it keeps the current behavior on 1.6-, throwing when fields != properties. 1.7+ get the correct properties handling.

Also, maybe there are better ways to get struct properties as a namedtuple?

[codecov-commenter]

Codecov Report

Merging #60 (16ba039) into master (dec3c28) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   98.16%   98.24%   +0.08%     
==========================================
  Files           3        3              
  Lines         109      114       +5     
==========================================
+ Hits          107      112       +5     
  Misses          2        2              

Impacted Files	Coverage Δ
src/ConstructionBase.jl	97.53% <100.00%> (+0.16%)	⬆️

---

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 dec3c28...16ba039. Read the comment docs.

[rafaqz]

Julia v1.6 is LTS and this package has ~ 500 dependants.

It worries me a lot dropping 1.6 support.

[aplavin]

It worries me a lot dropping 1.6 support.

Nothing gets dropped. Users on Julia 1.7+ get the bug fixed, users on 1.6- get a cleaner error instead of a confusing exception or silently wrong behavior.
All tests of this package and major dependants pass on all julia versions.

[jw3126]

LGTM (the downstream package tests hack needs to be removed before merge)

[aplavin]

@jw3126 as you asked about @generated more generally, please see the last commit: it simplifies two other constructors, getting rid of @generated. Doesn't look like it was necessary there in the first place...

[rafaqz]

@aplavin this is what I'm worried about

Well, the same function works, but slower

It has to be a compile time operation on 1.6

(This package is used in extremely performance critical code because it compiles away. If we add any cost at all to the methods here it will have very bad consequences, and effectively remove the possibility to use 1.6, even if it "still works")

[aplavin]

@rafaqz that comment of mine referred to an older version of this PR. Currently, there should be no performance regression.
For example, all Accessors.jl performance tests pass.

[jw3126]

I would like to merge this? Any objections?

[jw3126]

@aplavin can you merge master into this so the integration tests run with this PR? After they pass I will merge this PR!

[jw3126]

Thanks again @aplavin this PR is a huge improvement!

[ancapdev]

Just pulled this in to some code that now breaks.

While this solves the issue for anyone who wants to operate on custom properties, the scope of usability seems rather narrow as it breaks any type that has computed properties (trying to to call a constructor with all those properties as arguments) which you'd never want to set nor construct from.

[aplavin]

@ancapdev this PR just fixes a bug in ConstructionBase: earlier, getproperties returned properties, but used field names instead of property names. This makes little sense conceptually, and (I think) was implemented because older julia versions didn't allow making the proper behavior performant.

There has been work on introducing getfields in #54, sounds like fields may better suit your usecase than properties.
By the way, getproperties docs specifically mention that in your (computed) case it makes sense to define getproperties(::MyType) explicitly.

[jw3126]

@ancapdev thanks a lot this is valuable feedback for us. Is some of that code open source, I would like to take a look at it?

[ancapdev]

@aplavin Thanks, that's a good option, we could define getproperties directly. Our use case is indirect through Setfield.jl.

@jw3126 The affected code we have is not public, unfortunately, but it's nothing particularly complex. We simply had some structs with a few computed properties, and we were using Setfield.@set to update underlying fields on some of these objects.

[jw3126]

If you are only interested in Setfield.@set I would directly overload ConstructionBase.setproperties and not touch getproperties.

[ancapdev]

That also makes sense, thanks