feat: constructor mapping support for MapProperty attribute

[martinothamar]

Hi, thanks for this great library!

I had an issue where I tried to map a class to a record with primary constructor, where one of the property names were not matching, so I had to use the MapProperty attribute, this failed and I think I tracked down why. I included a couple of tests to illustrate the issue. The ClassesCanMapToPrimaryConstructorRecordsWhenUsingMapProperty tests failed, but it worked without MapProperty, so hopefully I've added the correct mapping code

[latonz]

Thank you for your contribution.
I reviewed your code and commented my feedback.
If you have any questions or points to discuss don't hesitate to comment 😊

[martinothamar]

I've incorporated most of the feedback I think. Sorry about the messy PR, this started out as debugging, was unsure if this was a me-bug or a mapperly-bug 😄

In terms of diagnostics, I think new ones are warranted right? Since the existing ones refer to init properties. I'm not sure what the process is around adding them to the shipped/unshipped text files.

I added the class test case as you suggested, and I'm wondering what the behavior should be now with the mapping attribute, since in the class case the the targets aren't really properties, but constructor arguments, and so they tend to not have the exact same name as the actual target properties (pascal vs camel case typically), and in some real world code the arguments don't cleanly map to properties at all.

I've still barely used this library so I'm not going to be able to have much good input on design here, but I guess one option is to limit the MapProperty support to records with primary constructor syntax only for now? In the class case where you want a single constructor with arguments, the manual configuration option would be source property -> target constructor arg? Thoughts?

[latonz]

No worries at all, thats what reviews are for! I'm glad you are contributing 😊 The implementation looks pretty good already!

Yap, new diagnostics are good. Also adding them to the released file is how we do it as we don't want to create release commits.

I think the current design is ok. To apply the MapProperty attribute to map to ctor parameters, one couldn't use the nameof operator but can still use the string overload. IMO thats also what should be done in the CanResolveToClassConstructorWithMapPropertyAttribute test case: [MapProperty(nameof(ThingClass.Id), "id2)].

[martinothamar]

Addressed the new comments now, and included the documentation in the document you suggested.

Did I get the diagnostics severity right? In that one should be error, the other warning?

I did find one "issue" when writing the documentation. When a target type has both a constructor and a settable public property, then it is both mapping the property to the constructor parameter as well as the public setter of the target property:

public class Car
{
    public string ModelName { get; set; }
}

public class CarDto
{
    // highlight-start
    public CarDto(string model)
    // highlight-end
    {
        ModelName = model;
    }

    public string ModelName { get; set; }
}

[Mapper]
public partial class CarMapper
{
    // highlight-start
    [MapProperty(nameof(Car.ModelName), "model")]
    // highlight-end
    public partial CarDto ToDto(Car car);
}

Generates the following

#nullable enable
namespace Riok.Mapperly.Testss
{
    public partial class CarMapper
    {
        public partial Riok.Mapperly.Testss.CarDto ToDto(Riok.Mapperly.Testss.Car car)
        {
            var target = new Riok.Mapperly.Testss.CarDto(car.ModelName);
            target.ModelName = car.ModelName;
            return target;
        }
    }
}

I'm not sure I define this a bug, in this specific case it's a noop, but there are cases where this can produce some unexpected results for sure

[martinothamar]

In terms of code formatting, I've enjoyed my experience with belav/csharpier. I use it across all projects using the MsBuild package in a Directory.Build.props file in my Mediator project and use it for various projects at work as well. Maybe that can be considered here as well

[latonz]

I think the behaviour you described is as expected (at least with the current implementation). Mapperly uses the target properties as the list of properties to map, and therefore doesn't know this property is already set via the ctor. I think this case should be rare though (no matching/manual matched ctor parameter but a matching setable property). And there is an easy fix for such cases by using the MapperIgnoreTargetAttribute.

Regarding the formatting: this codebase uses the dotnet built-in dotnet format command to check the format of the source code based on the editorconfig file. Until now this was sufficient... I didn't know csharpier and will check it out for sure. However, I think there need to be some crucial advantages over dotnet format to pull in another dependency.

Could you squash your commits together in one feature commit with a commit message according to the conventional commits specification (the commit message will land in the release notes) and rebase your branch onto the latest upstream main?

[martinothamar]

Updated commit msg and PR title and rebased.

Without going too off-topic here, the drawback with dotnet format that I found was that it didn't really deal with line length much (seems like that is still the case). Csharpier seems to handle everything, and is more prettier-like/opinionated.

[codecov-commenter]

Codecov Report

Merging #239 (9385086) into main (d39ace7) will increase coverage by 1.67%.
The diff coverage is 93.15%.

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   89.85%   91.53%   +1.67%     
==========================================
  Files          76       96      +20     
  Lines        2475     3095     +620     
  Branches      308      400      +92     
==========================================
+ Hits         2224     2833     +609     
+ Misses        181      180       -1     
- Partials       70       82      +12     

Impacted Files	Coverage Δ
...y/Descriptors/MappingBuilder/CtorMappingBuilder.cs	71.42% <0.00%> (-11.91%)	⬇️
...scriptors/MappingBuilder/NullableMappingBuilder.cs	95.45% <ø> (ø)
...ppings/PropertyNullAssignmentInitializerMapping.cs	66.66% <ø> (ø)
.../Riok.Mapperly/Descriptors/Mappings/TypeMapping.cs	100.00% <ø> (ø)
src/Riok.Mapperly/Helpers/SymbolExtensions.cs	84.21% <58.82%> (-10.79%)	⬇️
...der/NewInstanceObjectPropertyMappingBodyBuilder.cs	77.60% <77.60%> (ø)
...riptors/MappingBuilder/DictionaryMappingBuilder.cs	64.00% <78.57%> (ø)
...Riok.Mapperly/Descriptors/MappingBuilderContext.cs	97.61% <80.00%> (+0.11%)	⬆️
...scriptors/Mappings/UserImplementedMethodMapping.cs	85.71% <80.00%> (-7.15%)	⬇️
...tFactories/GenericTargetObjectFactoryWithSource.cs	80.00% <80.00%> (ø)
... and 76 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

🎉 This PR is included in version 2.7.0-next.1 🎉

The release is available on:

• GitHub release
• v2.7.0-next.1

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.7.0 🎉

The release is available on:

• GitHub release
• v2.7.0

Your semantic-release bot 📦🚀