Conversation
test: Run #2189
🎉 All tests passed!🔄 This comment has been updated |
Saancreed
left a comment
Saancreed
left a comment
There was a problem hiding this comment.
Would it be possible to report a diagnostic if an enum is declared without explicit values assigned?
And out of curiosity, how well does this handle enums with multiple members having the same value? Maybe it deserves some tests to showcase it.
| InProgress = 1, | ||
| Completed = 2, | ||
| Cancelled = 3, | ||
| Deleted = 4 |
There was a problem hiding this comment.
Question: would this still report a diagnostic even if Deleted was marked with ExcludeFromContractsGeneration? If yes, maybe it shouldn't?
| /// When applied, the enum will not be checked for consistency with its base enum. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Enum, Inherited = false, AllowMultiple = false)] | ||
| public sealed class IgnoreEnumDtoAttribute : Attribute { } |
There was a problem hiding this comment.
| public sealed class IgnoreEnumDtoAttribute : Attribute { } | |
| public sealed class IgnoreEnumDtoAttribute : Attribute; |
| /// When applied, the analyzer will not require a corresponding value in the base enum. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Field, Inherited = false, AllowMultiple = false)] | ||
| public sealed class IgnoreEnumValueAttribute : Attribute { } |
There was a problem hiding this comment.
| public sealed class IgnoreEnumValueAttribute : Attribute { } | |
| public sealed class IgnoreEnumValueAttribute : Attribute; |
| public class EnumDtoConsistencyAnalyzerTests : DiagnosticVerifier | ||
| { | ||
| private const string AttributeDefinitions = | ||
| @" |
There was a problem hiding this comment.
Why aren't you using """ for multiline strings? This isn't C# 10 anymore 😛
| InProgress = 2, | ||
| [LeanCode.CodeAnalysis.EnumValueCorresponds(Status.Completed)] | ||
| Completed = 3, | ||
| [LeanCode.CodeAnalysis.EnumValueCorresponds(Status.Cancelled)] |
There was a problem hiding this comment.
Do remember that this is something you'll never see in practice. Contracts projects won't have references do domain projects, so at best you can do something like
| [LeanCode.CodeAnalysis.EnumValueCorresponds(Status.Cancelled)] | |
| [LeanCode.CodeAnalysis.EnumValueCorresponds(3)] |
| } | ||
|
|
||
| // This handles cases where enums have different underlying types (byte, short, int, etc.) | ||
| if (value1.GetType() != value2.GetType()) |
There was a problem hiding this comment.
Does GetType actually return int, short, etc. here? Not TestEnum and TestEnumDTO? I thought it's necessary to use Enum.GetUnderlyingType
Patonymous
left a comment
Patonymous
left a comment
There was a problem hiding this comment.
Code looks good, I have some general questions and I'm also interested in answers to previous @Saancreed's comments
There was a problem hiding this comment.
I feel like tests concerning EnumValueCorrespondsAttribute are pretty similar, do we need all of them?
There was a problem hiding this comment.
Some helper functions here are the same as in EnumDtoConsistencyAnalyzer.cs, can't we reuse them?
| @@ -0,0 +1,78 @@ | |||
| namespace LeanCode.CodeAnalysis; | |||
There was a problem hiding this comment.
I think we can't keep these attributes in this project. LeanCode.CodeAnalysis is referenced as global package, which are private-by-default (i.e. only in dev/build), thus they must not be referenced in the code. And attributes inevitably will be.
There was a problem hiding this comment.
Would [Conditional("…")] on these attributes solve this perhaps? 🤔
There was a problem hiding this comment.
I think the condition would be hard to come up with :P
|
|
||
| private static INamedTypeSymbol? FindBaseEnum(INamedTypeSymbol dtoEnum, string baseEnumName) | ||
| { | ||
| var sameNamespaceEnum = dtoEnum |
There was a problem hiding this comment.
Can this even happen in real project? We will always compare Contracts enum with Domain enum, thus the namespace will be different.
| return FindEnumInCompilation(dtoEnum.ContainingAssembly.GlobalNamespace, baseEnumName); | ||
| } | ||
|
|
||
| private static INamedTypeSymbol? FindEnumInCompilation(INamespaceSymbol namespaceSymbol, string enumName) |
There was a problem hiding this comment.
This searches through namespaces, not compilations, the name is slightly misleading. Moreover, I don't think this will work - as far as I get how dtoEnum.ContainingAssembly works, it will only return the contracts assembly. And we have to find an enum in a completely different assembly. Moreover, one that is not referenced by the contracts at all.
I wonder if it is possible to implement this check. Contracts don't know anything about the domain, the domain is only (happens to be) in the same solution. Roslyn does not work on solutions, it works on assemblies/projects. Thus, it cannot know about the domain enum. Or am I missing something?
@Saancreed , I think you are the most well-versed in Roslyn. Is my thinking right?
There was a problem hiding this comment.
@Saancreed, I think you are the most well-versed in Roslyn.
Uhm, citation needed? 🙈
Is my thinking right?
I also think it's not possible as long as we talk in terms of analysing just contracts or just domain projects. On the other hand, at some point we'll be building some project that references both, at which point I think it should be possible to do a pass over all known/referenced types when both contract and domain enums are visible. Maybe this should be the point where we run the analyzer and report violations.
But of course that makes [Conditional("…")] idea unviable 🙁
There was a problem hiding this comment.
That... might work! Thx!
4 participants
Should I add a helper with common methods (e.g. FindBaseEnum, FindEnumInCompilation, GetExcludedMembers andAreEnumValuesEqual)?