[API Proposal]: Make DependencyModel.Dependency readonly and implement IEquatable<> #56082
Labels
Milestone
Comments
hrrrrustic
added
the
api-suggestion
dotnet-issue-labeler
bot
added
area-DependencyModel
untriaged
|
Tagging subscribers to this area: @eerhardt Issue DetailsBackground and motivationThe current implementation looks like namespace Microsoft.Extensions.DependencyModel
{
public struct Dependency
{
// for readonly proposal
public string Name { get; }
public string Version { get; }
// for IEquatable<> proposal
public bool Equals(Dependency other);
public override bool Equals(object obj); // common pattern with is T t && Equals(t)
public override int GetHashCode();
}
}So, all required members for IEquatable<> exist and properties aren't settable API Proposalnamespace Microsoft.Extensions.DependencyModel
{
- public struct Dependency {}
+ public readonly struct Dependency : IEquatable<Dependency> {}
}The main point here is that nothing will be changed in the implementation as it already satisfied for these changes API UsageThere are already some usages in HashSet<> where new and some in dotnet/sdk Also RisksNone?
|
|
Thanks for the suggestion. Seems reasonable to me. |
eerhardt
added
api-ready-for-review
|
Looks good as proposed. namespace Microsoft.Extensions.DependencyModel
{
- public struct Dependency {}
+ public readonly struct Dependency : IEquatable<Dependency> {}
} |
bartonjs
added
api-approved
|
@bartonjs, @terrajobst, we explicitly disable CA1066 because we have a bunch of types that don't do this "correctly": runtime/eng/CodeAnalysis.src.globalconfig Lines 154 to 155 in 03601a7 Should we holistically make a pass through all of them rather than continuing with these one-offs? |
ghost
added
the
in-pr
|
Hmm, maybe instead of disabling that rule we should just pragma suppress the files that host these types? |
|
We could... for reference, here are the resulting warnings if it's enabled currently: 68 Warning(s) |
|
68 types doesn't seem too bad. But yeah. |
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Background and motivation
Similar to #31996 and #2127
The current implementation looks like
So, all required members for IEquatable<> exist and properties aren't settable
API Proposal
namespace Microsoft.Extensions.DependencyModel { - public struct Dependency {} + public readonly struct Dependency : IEquatable<Dependency> {} }The main point here is that nothing will be changed in the implementation as it already satisfied for these changes
API Usage
There are already some usages in HashSet<> where new
EqualityComparerwill be usedruntime/src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextWriter.cs
Line 266 in 57bfe47
and some in dotnet/sdk
Also
Dependencyis public, so it might be useful for someone else 😄Risks
None?
The text was updated successfully, but these errors were encountered: