Add support for From and To method conversions (#1117)
#1616
Conversation
TonEnfer
force-pushed
the
#1117_Support_From_and_To_static_factory_methods
branch
from
22527b1
to
2d82323
Compare
| //checks for backward compatibility | ||
|
|
||
| if (IsDateTimeToDateOnlyConversion(ctx)) | ||
| { | ||
| if (!ctx.IsConversionEnabled(MappingConversionType.DateTimeToDateOnly)) | ||
| return null; | ||
| } | ||
| else if (IsDateTimeToTimeOnlyConversion(ctx)) | ||
| { | ||
| if (!ctx.IsConversionEnabled(MappingConversionType.DateTimeToTimeOnly)) | ||
| return null; | ||
| } | ||
| else | ||
| { | ||
| if (!ctx.IsConversionEnabled(MappingConversionType.StaticConvertMethods)) | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Nit: it could make sense to extract this into a method IsConversionEnabled(ctx)
| && ctx.Types.DateOnly != null | ||
| && ctx.Target is INamedTypeSymbol namedSymbol | ||
| && SymbolEqualityComparer.Default.Equals(namedSymbol, ctx.Types.DateOnly) | ||
| || true; |
There was a problem hiding this comment.
Is this || true really required? 🤔
There was a problem hiding this comment.
Oops. I have no idea where this came from 🤦
| var methodCandidates = ctx | ||
| .SymbolAccessor.GetAllMethods(ctx.Source, methodName) | ||
| .Where(x => x is { IsStatic: false, ReturnsVoid: false, IsAsync: false, Parameters.Length: 0 }) | ||
| .ToArray(); |
There was a problem hiding this comment.
Usually we opt for ToList in similar scenarios (AFAIK it performs a little better than ToArray if the source count is unknown). Is there a reason you used ToArray? The same question applies at several places in this PR.
There was a problem hiding this comment.
The only reason I chose ToArray is because the list is not mutable and should not be mutable further down the code.
I'll change it to ToList since it works better.
| if (!ctx.Source.IsArrayType(out var arrayType)) | ||
| yield return $"{from}{ctx.Source.Name}"; | ||
| else | ||
| yield return $"{from}{arrayType.ElementType.Name}Array"; |
There was a problem hiding this comment.
Wouldn't it be logical to also try $"{create}{from}{ctx.Source.Name}"?
There was a problem hiding this comment.
I think you are right, I will add this
| else | ||
| { | ||
| if (ctx.Source is INamedTypeSymbol { TypeArguments.Length: 1 } namedTypeSymbol) |
There was a problem hiding this comment.
This could be a simple else if clause, couldn't it?
|
|
||
| private static bool FilterParameterType(IParameterSymbol parameter, ITypeSymbol targetType) | ||
| { | ||
| if (SymbolEqualityComparer.Default.Equals(parameter.Type, targetType)) |
There was a problem hiding this comment.
Shouldn't this include nullability?
| if (parameter.Type.IsArrayType(out var arrayTypeSymbol)) | ||
| { | ||
| return SymbolEqualityComparer.Default.Equals( | ||
| arrayTypeSymbol.ElementType, | ||
| targetIsArray ? targetArrayTypeSymbol!.ElementType : targetType | ||
| ); | ||
| } |
There was a problem hiding this comment.
This early return could happen before the enumerable check of the target, couldn't it?
| if (parameter.Type.IsArrayType(out var arrayTypeSymbol)) | ||
| { | ||
| return SymbolEqualityComparer.Default.Equals( | ||
| arrayTypeSymbol.ElementType, | ||
| targetIsArray ? targetArrayTypeSymbol!.ElementType : targetType | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is this section really required? If the target type is an array and the source type matches it, it should already be handled... If it is some other collection type it is handled later on anyway...
| return targetIsEnumerable | ||
| ? parameter.Type.AllInterfaces.Intersect(targetType.AllInterfaces, SymbolEqualityComparer.Default).Any() | ||
| : SymbolEqualityComparer.Default.Equals(((INamedTypeSymbol)parameter.Type)?.TypeArguments[0], targetType); |
There was a problem hiding this comment.
Couldn't SymbolAccessor.CanAssign be used here?
|
|
||
| if (!nonNullableTarget.IsArrayType(out var arrayType)) | ||
| { | ||
| return $"To{nonNullableTarget.Name}"; |
There was a problem hiding this comment.
TBD (same for the static one): Would it make sense to also support language keyword names (e.g. ToInt instead of only ToInt32 etc.)?
There was a problem hiding this comment.
I guess that would make sense, but I don't know how I would get language keyword names. Could you give me a hint?
There was a problem hiding this comment.
I don't think there is an easy way to do this, you may need to handle it with a dictionary or a switch...
Add support for
FromandTomethod conversions (#1117)Description
Despite the discussion in the issue, the ToString mapping builder remains untouched as it contains a lot of logic.
Instead, the new mapping builder eliminates the mapping to
string.The previously existing mapping builders for
DateTimemapping have been removed, as their role is now performed by the new mapping builder for static methods.However,
MappingConversionTypehas only been extended, the previously existingMappingConversionType.DateTimeToDateOnlyandMappingConversionType.DateTimeToTimeOnlyare left for backward compatibility, their processing has been moved to the new mapping builderFixes #1117
Checklist