riok / mapperly

A .NET source generator for generating object mappings. No runtime reflection.
https://mapperly.riok.app
Apache License 2.0
2.95k stars 147 forks source link

MapperDefaults configuration API for MSBuild (csproj, Directory.Build.props, ...) #1581

Open pomianowski opened 3 weeks ago

pomianowski commented 3 weeks ago

Is your feature request related to a problem? Please describe. The default enum mapping option uses values that represent some items in some order. The ByName option, on the other hand, seems to make more business sense.

Example:

gRPC API model

public enum AttachmentVisibilityGenerated {
  Unknown,
  User,
  System,
}

domain API model

public enum AttachmentVisibility {
  User,
  System,
}

Default extension class like

[Mapper]
internal static partial class AttachmentVisibilityExtensions
{
    public static partial AttachmentVisibility ConvertToDomainModel(
        AttachmentVisibilityGenerated from
    );
}

generates:

// <auto-generated />
#nullable enable
namespace MyNamespace
{
    internal static partial class AttachmentVisibilityExtensions
    {
        [global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "4.1.0.0")]
        public static partial global::AttachmentVisibility ConvertToDomainModel(this global::AttachmentVisibilityGenerated from)
        {
            return (global::AttachmentVisibility)from;
        }
    }
}

, and will return User, instead of Unknown.

Describe the solution you'd like

By default, the enum should map 1:1 by name and throw an exception if the value is not present.

[Mapper]
internal static partial class AttachmentVisibilityExtensions
{
    public static partial AttachmentVisibility ConvertToDomainModel(
        AttachmentVisibilityGenerated from
    );
}

should generate

// <auto-generated />
#nullable enable
namespace MyNamespace
{
    internal static partial class AttachmentVisibilityExtensions
    {
        [global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "4.1.0.0")]
        public static partial global::AttachmentVisibility ConvertToDomainModel(this global::AttachmentVisibilityGenerated from)
        {
            return from switch
            {
                global::AttachmentVisibilityGenerated.User => global::AttachmentVisibility.User,
                global::AttachmentVisibilityGenerated.System=> global::AttachmentVisibility.System,
                _ => throw new System.ArgumentOutOfRangeException(nameof(from), from, "The value of enum AttachmentVisibility is not supported"),
            }
        }
    }
}

like when using

[Mapper(
    EnumMappingStrategy = EnumMappingStrategy.ByName,
    RequiredMappingStrategy = RequiredMappingStrategy.Target
)]

Describe alternatives you've considered

Could use already available

[assembly: MapperDefaults(EnumMappingStrategy = EnumMappingStrategy.ByName)]

, but the above is not very convenient for split dlls and is easily forgotten

Or add new global default switch to csproj like:

<PropertyGroup>
  <DefineConstants>$(DefineConstants);MAPPERLY_ENUM_MAPPING_BY_NAME</DefineConstants>
</PropertyGroup>

or

<PropertyGroup>
  <MapperlyEnumMappingByName>true</MapperlyEnumMappingByName>
</PropertyGroup>

, could be added to Directory.Build.props.

Additional context It seems to me that much more often the name of an enumeration in a business concourse is more important than its number.

325

336

337

related

latonz commented 3 weeks ago

Can you provide evidence that ByName is more common than ByValue? In the projects I work I'd say its about 60% ByValue. Regarding your <MapperlyEnumMappingByName>true</MapperlyEnumMappingByName> csproj proposal: I think if we want to introduce a MSBuild configuration API for Mapperly we should implement this for all MapperDefaultsAttribute options. It would then probably more look like <MapperlyEnumMappingStrategy>ByName</MapperlyEnumMappingStrategy>. Not sure if we really want to add the complexity of an additional configuration API, just not sure what use-cases it addresses and what value it provides. However, I'm open if one want's to experiment a bit on how such an API could be implemented.

pomianowski commented 3 weeks ago

Hey @latonz, thanks for your reply. When it comes to ByName in my business it is mostly the other way around. We have huge enums defining business customer types, transaction types, business process types etc. We often map from grpc > dtos > domain > projections > dtos > rest.

Sometimes there are redundant values, such as for API purposes. And sometimes a given business model supports only a few values, so it's always about the name.

The msbuild property:

<MapperlyEnumMappingStrategy>ByName</MapperlyEnumMappingStrategy>

Looks cool to me. We have dozens of repositories, some having as many as 1,000 projects. We can either statically parse the Directory.Build.props and .editorconfig or synchronize settings between them.

For me, it would probably look like this

<Project>
  <PropertyGroup>
    <MapperlyAllowOptionOverride>false</MapperlyAllowOptionOverride>
    <MapperlyRequiredMappingStrategy>Target</MapperlyRequiredMappingStrategy>
    <MapperlyEnumMappingStrategy>ByName</MapperlyEnumMappingStrategy>
    <MapperlyEnumNamingStrategy>MemberName</MapperlyEnumNamingStrategy>
    <MapperlyPropertyNameMappingStrategy>CaseSensitive</MapperlyPropertyNameMappingStrategy>
    <MapperlyEnumMappingIgnoreCase>false</MapperlyEnumMappingIgnoreCase>
    <MapperlyThrowOnPropertyMappingNullMismatch>true</MapperlyThrowOnPropertyMappingNullMismatch>
    <MapperlyThrowOnMappingNullMismatch>true</MapperlyThrowOnMappingNullMismatch>
  </PropertyGroup>
</Project>

I would also suggest something like MapperlyAllowOptionOverride similar to EnablePackageVersionOverride for NuGet packages.

In my opinion, we do not add complexity for the user, if the use of MsBuild properties is optional. Whoever wants can just use it.

latonz commented 3 weeks ago

That may be true for your business domain, but is it true for most business domains? I'm not really sure.

There is little increase in complexity for the user of Mapperly, but the complexity of Mapperly itself increases quite a bit, resulting in more maintenance required. As I said, I'm happy if a contributor wants to experiment with an MSBuild configuration API for Mapperly and provide a concept for how to implement it, but it's not a top priority for me.

pomianowski commented 3 weeks ago

Okay, thanks for your opinion and reply. As always, the answer is, it depends. I'll try to take a look at the code when I have some free time