ConstructionBase integration #99
Conversation
|
Hm, not clear why 1.6 is failing. Here's the error msg: |
|
|
||
| a = A91V1(; a=1) | ||
| a2 = @set a.a = 2 | ||
| @test a2.a == 2 |
There was a problem hiding this comment.
Why are these tests needed? Seems like the later testset covers this. At the very least this should be made into a separate testset inside this module
There was a problem hiding this comment.
just to make sure that the ConstructionBase name is interpolated correctly as well, since we generate code with $ConstructionBase. in teh macro
There was a problem hiding this comment.
Okay, would have been nice to have this as a separate testcase inside the submodule as mentioned. I'll leave this unresolved
| nt = ConstructionBase.getproperties(r) | ||
| nt_new = merge(nt, patch) | ||
| ConstructionBase.check_patch_properties_exist(nt_new, nt, r, patch) | ||
| args = Tuple(nt_new) # old julia inference prefers if we wrap in Tuple | ||
| return ConstructionBase.constructorof(R)(args...) |
There was a problem hiding this comment.
At first I was curious about improving performance as this converts to a NamedTuple first. As this is also what Tables.rowmerge already does this seems perfectly fine.
There was a problem hiding this comment.
yeah, I was starting to go down that rabbit hole and eventually decided this was probably good enough...
Co-authored-by: Curtis Vogt <[email protected]>
Due to the possibility of parametric record types, this is a bit more complex than just creating a namedtuple, so I opted to include a method for each
@versioninside the outer constructors block.There's an unfortunate bit of copy-paste required from ConstructionBase due to how ConstructionBase checks for propertynames vs. fieldnames in versions before 1.7; that can be removed if we stop supporting 1.6.
closes #97