Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify Format and Culture when converting to strings #887

Closed
ialexj opened this issue Nov 10, 2023 · 7 comments · Fixed by #929
Closed

Specify Format and Culture when converting to strings #887

ialexj opened this issue Nov 10, 2023 · 7 comments · Fixed by #929
Labels
enhancement New feature or request

Comments

@ialexj
Copy link

ialexj commented Nov 10, 2023

Is your feature request related to a problem? Please describe.
Related to the more generic request in #886, the most common use case would be specific string formatting per property.

Describe the solution you'd like

Being able to specify the string format and culture on a per-property basis would go a long way. This could be achieved by augmenting the [MapProperty] attribute:

// assume mapping all properties to string
[Mapper]
static partial class DateModelMapper
{
    [MapProperty(nameof(Model.Date), nameof(Dto.Date), StringFormat="d")]
    [MapProperty(nameof(Model.Time), nameof(Dto.Time), StringFormat="t")]
    [MapProperty(nameof(Model.Money), nameof(Dto.Money), StringFormat="c")]
    public static partial Dto ToDto(this Model model);
}

When considering string formatting, being able to control the culture is crucial. It's also useful to be able to specify it at a more global level. As such, I propose adding two optional properties Culture and CultureName to [MapProperty], [Mapper] and [MapperDefaults].

Culture is a:

public enum MapperCulture { Default, Current, Invariant }

With the following values and behavior on MapProperty:

  • Default:
    • If CultureName is specified:
      • Look for a property (instance or static) of type CultureInfo named by the value in CultureName. If found, use it.
      • If a property is not found, use ToString(CultureInfo.GetCultureInfo(CultureName)).
    • If CultureName is not specified:
      • Check [Mapper] if a non-Default Culture or CultureName is set, and apply same logic as above.
      • If not, and the class contains exactly one property (static or instance) of type CultureInfo, use it.
    • If none of the above match, try MapperDefaults and apply the same logic.
    • If MapperDefaults.Culture is also set to Default, use CultureInfo.CurrentCulture.
  • Current: Use ToString(CultureInfo.CurrentCulture).
  • Invariant: Use ToString(CultureInfo.InvariantCulture)

This would allow for pretty flexible scenarios:

[Mapper(CultureName="en-GB")]
static partial class Mapper
{
    static readonly CultureInfo EurCulture = CreateCurrentCultureWithCurrencySymbol("€");

    [MapProperty(nameof(MovementDate), nameof(MovementDate), StringFormat = "D")] // uses en-GB
    [MapProperty(nameof(AuditDateTime), nameof(AuditDateTime), StringFormat = "G", Culture = MapperCulture.Invariant)] // uses InvariantCulture
    [MapProperty(nameof(Model.MoneyInEur), nameof(Dto.MoneyInEur), StringFormat="c", CultureName=nameof(EurCulture))]
    [MapProperty(nameof(Model.MoneyInUsd), nameof(Dto.MoneyInUsd), StringFormat="c", CultureName="en-US")]
    public static partial Dto ToDto(this Model model);
}

In instance-based scenarios, this would also allow the culture to be set by the user.

class Mapper
{
  public CultureInfo MapperCulture { get; set; } = CultureInfo.CurrentCulture;

  public static partial Dto ToDto(this Model model);
}

Describe alternatives you've considered
Setting the culture on a global per-thread basis. Not always helpful, because in certain scenarios I need to map to formats expected by heavily localized APIs, or for generating report data in specific formats.

@latonz
Copy link
Contributor

latonz commented Nov 13, 2023

Thank you for opening this issue. These would be useful additions. I think we should split it up into two issues, one for the string format and one for the culture. I like the string format API. The culture info seems a bit complex and I'm not sure if we want to add this complexity to the API surface. I like the property approach, what do you think about a simple property/field based culture API like this:

  • Explicitly annotate culture info properties/fields to be used by Mapperly with an attribute MapperCultureAttribute (this aligns with other Mapperly APIs such as object factory and external mappers). This gets used as a default by the mapper.
  • To specify a different culture for a subset of properties, the MapPropertyAttribute could be extended by a new string Culture property which references a culture info property/field.

Example

An example mapper which uses de-CH for all mappings except for Entity.A to Entity.B where it uses de-DE.

[Mapper]
partial class CarMapper
{
    [MapperCulture]
    private readonly CultureInfo _defaultCulture = CultureInfo.GetCultureInfo("de-CH");

    [MapperCulture]
    private readonly CultureInfo _secondaryCulture = CultureInfo.GetCultureInfo("de-DE");

    [MapProperty("A", "B", Culture = nameof(_secondaryCulture))]
    public Dto MapToDto(Entity entity);
}

@ialexj
Copy link
Author

ialexj commented Nov 14, 2023

Thank you for the feedback!

I realize now that maybe it's better to accept the IFormatProvider interface, rather than CultureInfo, since that's what ToString actually requires.

Only issue I see with your approach is a cardinality mismatch - there's only one default, but you can have more than one instance annotated with MapperCultureAttribute, and it's not obvious what that would mean.

If I specify a culture on MapProperty that's not annotated, would that be an error? If not, then why annotate at all?

(I don't know enough about source generators, do you need to annotate all the members you expect to use for codegen?)

An attribute on the Mapper can be limited to exactly one, and you're still adding a property to MapProperty anyway.

I admit the enum seems a bit superfluous. I'd be happy with just the Default logic, and only one Culture (or maybe FormatProvider?) property on Mapper/MapProperty.

@latonz
Copy link
Contributor

latonz commented Nov 14, 2023

Thank you for your input! We should definitely go for IFormatProvider. Therefore the new proposal which should also address the other points brought up by you:

  • Add a new attribute named DefaultFormatProviderAttribute
  • Only a single appliance of this attribute is allowed per mapper
  • It can only be applied to a field or getable property of type implementing IFormatProvider
  • The attributed property/field acts as the default IFormatProvider
  • Other IFormatProviders can be referenced by a new string FormatProvider property on the MapPropertyAttribute
  • An error diagnostic should be emitted if the DefaultFormatProviderAttribute is applied multiple times, the field/property does not have a getter, does not return type IFormatProvider or a FormatProvider reference of a MapPropertyAttribute cannot be resolved.

Example

An example mapper which uses de-CH for all mappings except for Entity.Value1 to Entity.Value1 where it uses de-DE.

record Entity(DateTime Value0, int Value1);
record Dto(string Value0, string Value1);

[Mapper]
partial class CarMapper
{
    [DefaultFormatProvider]
    private readonly CultureInfo _defaultCulture = CultureInfo.GetCultureInfo("de-CH");
    private readonly IFormatProvider _secondaryCulture = CultureInfo.GetCultureInfo("de-DE");

    [MapProperty("Value1", "Value1", FormatProvider = nameof(_secondaryCulture), StringFormat = "dd.mm.yyyy"]
    public Dto MapToDto(Entity entity);
}

// simplified generated code of the MapToDto implementation:
target.Value0 = entity.Value0.ToString(_defaultCulture);
target.Value1 = entity.Value1.ToString("dd.mm.yyyy", _secondaryCulture);

@ialexj
Copy link
Author

ialexj commented Nov 14, 2023

Just wondering why not like this:

record Entity(DateTime Value0, int Value1);
record Dto(string Value0, string Value1);

[Mapper]
[DefaultFormatProvider(nameof(_defaultCulture))]
partial class CarMapper
{
    private readonly CultureInfo _defaultCulture = CultureInfo.GetCultureInfo("de-CH");
    private readonly CultureInfo _secondaryCulture = CultureInfo.GetCultureInfo("de-DE");

    [MapProperty("Value1", "Value1", FormatProvider = nameof(_secondaryCulture), StringFormat = "dd.mm.yyyy"]
    public Dto MapToDto(Entity entity);
}

Since you can just set [AttributeUsage(AttributeTargets.Class, AllowMultiple = false)] to guarantee a single target, rather than writing an analyzer?

But it does accomplish the same task, so overall I'm happy.

@latonz
Copy link
Contributor

latonz commented Nov 20, 2023

During the implementation I thought again on the design and switched back to my first proposal with an additional Default flag. The following points lead to this:

  • The attribute allows to opt in for Mapperly to also validate the signature of a format provider which should help diagnosing why a format provider can't be found / isn't picked up by Mapperly
  • The additional default flag allows easily to set a default format provider while it is still possible to have no default provider (no format provider is used unless one is explicitly set for a property)

A first draft of the PR is available in #929

Copy link

🎉 This issue has been resolved in version 3.3.0-next.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

🎉 This issue has been resolved in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants