-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Suppress warnings on uninitialized DbSet properties #26795
Conversation
8550edf
to
bcffe52
Compare
bcffe52
to
a0b2f87
Compare
/cc @Youssef1313 |
private static readonly SuppressionDescriptor _suppressUninitializedDbSetRule = new( | ||
id: "SPR1001", | ||
suppressedDiagnosticId: "CS8618", | ||
justification: "DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this being non-localized intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that nothing else in EF Core is localized at this point.
It's true we manage exception strings via resx, but those sometimes get reused (or asserted upon in tests). I'd rather extract these strings out based on need rather than proactively, though can do that if the team prefers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roji We have so far taken the approach that everything (possible) is localization ready. I'd prefer we keep doing that, but we can discuss with the team.
test/EFCore.Analyzers.Tests/UninitializedDbSetDiagnosticSuppressorTest.cs
Show resolved
Hide resolved
static bool InheritsFrom(ITypeSymbol typeSymbol, ITypeSymbol baseTypeSymbol) | ||
{ | ||
var baseType = typeSymbol.BaseType; | ||
while (baseType is not null) | ||
{ | ||
if (baseType.Equals(baseTypeSymbol, SymbolEqualityComparer.Default)) | ||
{ | ||
return true; | ||
} | ||
|
||
baseType = baseType.BaseType; | ||
} | ||
|
||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to look at the full hierarchy, not only the parent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - the user's DbContext type can be lower down the hierarchy, rather than directly extend DbContext. A good example is ASP.NET Identity, where user context types can extend IdentityDbContext, which itself extends DbContext. The moment DbContext is somewhere in the base type list, we know its constructor will get invoked, and therefore the DbSets will get populated.
9def9d5
to
da6b358
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
5acc0e3
to
c5f0ae7
Compare
@ajcvickers you may want to give my localization commit a quick look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Closes #21608