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

[API Proposal]: Add IComparable to IPAddress and PhysicalAddress classes #56627

Closed
maknapp opened this issue Jul 30, 2021 · 4 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Milestone

Comments

@maknapp
Copy link

maknapp commented Jul 30, 2021

Background and motivation

I am suggesting this mainly due to changes in efcore (see dotnet/efcore#23775). In short, efcore v5+ requires unique indexes to implement IComparable, preventing IPAddress and PhysicalAddress from being used as unique indexes. This is blocking me from updating some projects beyond .NET Core 3.1.

This has been suggested before for IPAddress (see #28964). A quick search shows many people looking to sort IPAddress and resorting to System.Version.Parse on the string, showing it is a fairly common need. The need for PhysicalAddress sorting seems less common.

API Proposal

namespace System.Net.NetworkInformation
{
    public class PhysicalAddress : IComparable<PhysicalAddress>
    {
        public int CompareTo(PhysicalAddress other);
    }
}

namespace System.Net
{
    public class IPAddress : IComparable<IPAddress>
    {
        public int CompareTo(IPAddress other);
    }
}

API Usage

var x = new[]
{
    "192.168.0.1",
    "0:0:0:0:0:FFFF:C0A8:0018",
    "192.168.0.1",
    "192.168.0.24",
    "2001:0db8:85a3:0000:0000:8a2e:0370:7334",
    "::f",
    "0:0:0:0:0:FFFF:0C05:2842",
    "192.168.2.1",
    "192.168.0.10",
    "192.168.0.1",
    "2001:0db8:85a3:0000:0000:8a2e:0370:7334"
};

var y = x.Select(x => IPAddress.Parse(x)).OrderBy(x => x);

foreach(var item in y)
{
    Console.WriteLine(item);
}

// 192.168.0.1
// 192.168.0.1
// 192.168.0.1
// 192.168.0.10
// 192.168.0.24
// 192.168.2.1
// ::f
// ::ffff:12.5.40.66
// ::ffff:192.168.0.24
// 2001:db8:85a3::8a2e:370:7334
// 2001:db8:85a3::8a2e:370:7334

Risks

I do not see any breaking changes or changes in performance.

@maknapp maknapp added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 30, 2021
@lambdageek lambdageek added the untriaged New issue has not been triaged by the area owner label Jul 30, 2021
@huoyaoyuan
Copy link
Member

Generally the BCL shouldn't be modified to fit EF Core. They should fit BCL.

Does specification of network addresses mention sorting?

@ghost
Copy link

ghost commented Jul 31, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

I am suggesting this mainly due to changes in efcore (see dotnet/efcore#23775). In short, efcore v5+ requires unique indexes to implement IComparable, preventing IPAddress and PhysicalAddress from being used as unique indexes. This is blocking me from updating some projects beyond .NET Core 3.1.

This has been suggested before for IPAddress (see #28964). A quick search shows many people looking to sort IPAddress and resorting to System.Version.Parse on the string, showing it is a fairly common need. The need for PhysicalAddress sorting seems less common.

API Proposal

namespace System.Net.NetworkInformation
{
    public class PhysicalAddress : IComparable<PhysicalAddress>
    {
        public int CompareTo(PhysicalAddress other);
    }
}

namespace System.Net
{
    public class IPAddress : IComparable<IPAddress>
    {
        public int CompareTo(IPAddress other);
    }
}

API Usage

var x = new[]
{
    "192.168.0.1",
    "0:0:0:0:0:FFFF:C0A8:0018",
    "192.168.0.1",
    "192.168.0.24",
    "2001:0db8:85a3:0000:0000:8a2e:0370:7334",
    "::f",
    "0:0:0:0:0:FFFF:0C05:2842",
    "192.168.2.1",
    "192.168.0.10",
    "192.168.0.1",
    "2001:0db8:85a3:0000:0000:8a2e:0370:7334"
};

var y = x.Select(x => IPAddress.Parse(x)).OrderBy(x => x);

foreach(var item in y)
{
    Console.WriteLine(item);
}

// 192.168.0.1
// 192.168.0.1
// 192.168.0.1
// 192.168.0.10
// 192.168.0.24
// 192.168.2.1
// ::f
// ::ffff:12.5.40.66
// ::ffff:192.168.0.24
// 2001:db8:85a3::8a2e:370:7334
// 2001:db8:85a3::8a2e:370:7334

Risks

I do not see any breaking changes or changes in performance.

Author: maknapp
Assignees: -
Labels:

api-suggestion, area-System.Net, untriaged

Milestone: -

@geoffkizer
Copy link
Contributor

It seems reasonable to add IEquatable<T> in these cases, though in general that shouldn't be necessary -- generic collection types don't require this in general.

It seems really weird to add IComparable<T> in these cases, since there is no well defined ordering and so whatever we did would be arbitrary.

@ManickaP
Copy link
Member

ManickaP commented Aug 3, 2021

Triage: IComparable doesn't make sense here, we shouldn't introduce a new interface to a type to solve a particular EF Core issue. We might consider IEquatable in a new issue.

@ManickaP ManickaP closed this as completed Aug 3, 2021
@roji roji removed the untriaged New issue has not been triaged by the area owner label Aug 3, 2021
@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Projects
None yet
Development

No branches or pull requests

8 participants