fix: remove unneeded null conditional initializer #705
Conversation
Codecov Report
@@ Coverage Diff @@
## main #705 +/- ##
==========================================
- Coverage 91.45% 90.86% -0.60%
==========================================
Files 214 180 -34
Lines 7149 6325 -824
Branches 867 809 -58
==========================================
- Hits 6538 5747 -791
+ Misses 406 389 -17
+ Partials 205 189 -16
... and 107 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
TimothyMakkison
force-pushed
the
null_init
branch
3 times, most recently
from
4d0514a to
8b522bf
Compare
| @@ -42,6 +42,17 @@ private void AddNullMemberInitializers(IMemberAssignmentMappingContainer contain | |||
| if (!nullablePath.Member.CanSet) | |||
| continue; | |||
|
|
|||
| // if member is initialised with a non null value then skip | |||
| if ( | |||
There was a problem hiding this comment.
Wouldn't it be easier to keep a set of mapped source members (in SetSourceMemberMapped) and perform a simple lookup here? This way we wouldn't need to make the information public on the mapping.
There was a problem hiding this comment.
Wouldn't it be easier to keep a set of mapped source members (in SetSourceMemberMapped) and perform a simple lookup here?
Tried something similar but realised it would cause errors. If the source value is transformed we'd lose the type info ie target.Value = UserDefinedDoggyMapper(source.Value), here source.Value may not be nullable but the user defined mapper might return a null value
Yeah, I'm not a fan of exposing the properties either.
There was a problem hiding this comment.
But even in this case, we shouldn't initialize the property afterwards, should we? Wouldn't this be strange? It probably is the case today, but I don't think this makes a lot of sense.
There was a problem hiding this comment.
I'm not sure what you mean, could you rephrase this please?
FYI this PR doesn't change the order of member instantiation. It is still possible to null initialise before a member is set. All this PR does is omit a null initializer if the source value is not nullable.
There was a problem hiding this comment.
Sorry for my bad english... I thought Mapperly should not generate a ??= new(); if previously another mapping assigned a value to this property. However, on a second thought I think we should keep the current behaviour. But isn't the described approach possible by simply adding a parameter to a Set...MemberMapped method, whether the member is nullable or not?
There was a problem hiding this comment.
Have you pushed it in a branch so I can quickly look into it? 😊 Thank you!
This branch should have the changes as commit 99afa22. Do you want me to create a separate branch?
Note that I didn't remove my original modifications.
There was a problem hiding this comment.
Thanks for the commit. I tried a little around on the provided commit and came up with the following solution (available here):
- I exposed a
MemberTypeMappingfrom theIMemberAssignmentMapping SetTargetNonNullInitializedIfNonNullableadded toMembersContainerBuilderContextto keep track of the non-nullable initialized target paths
With this the correct nullable initializers were applied. However, under some conditions the initializers were emitted after a nested mapping. I fixed this with:
- Introducing
IMemberAssignmentMappingOrContainerto have a common interface forIMemberAssignmentMappingContainerandIMemberAssignmentMapping - Introducing an additional
List<IMemberAssignmentMappingOrContainer>to keep the containers and mappings ordered - Adjusted the
MemberAssignmentMappingContainer.Buildto use the ordered list as source
I'm not super happy with this solution, but IMO it still exposes less information/complexity than TryGetMemberMapping. WDYT?
There was a problem hiding this comment.
Sound good 👍
Does this mean this PR is no longer needed!?
There was a problem hiding this comment.
I've rebased this pr onto a recent branch with the changes, it doesn't appear to have improved.
Any idea of what to do next? The original hack of exposing the mapping worked, aside from that I can't remember what else I tried 😅
There was a problem hiding this comment.
Since I'm not really happy with the solutions discussed (and you don't seem to be either), I'm thinking about whether we should just remove the initialisation of these nullable values with the next major release/breaking change.
|
@TimothyMakkison any updates on this? Are you still working on this? 😊 Don't worry but otherwise I'd probably schedule this feature to be done by someone else... |
TimothyMakkison
force-pushed
the
null_init
branch
from
8b522bf to
99afa22
Compare
Really sorry, should be able to fix this and the unmapped property warning PR. I'll need to install the preview before doing the unsafe accessors Haven't investigated the issue, but it looks like my tests are failing. I'll look into this in the morning. |
TimothyMakkison
force-pushed
the
null_init
branch
from
99afa22 to
7fd4711
Compare
Description
Add check to only insert a null initializer if the root value is uninitialized or initialized by a nullable type.
tbh I feel like mapperly shouldn't insert a null initializer if the root is mapped. Having a nullable path seems like an error and this obscures it.
Partially fixes #640
Checklist