Skip to content

Document EIIs of integer types (IComparable/IConvertible) #3499

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

Merged

Conversation

carlossanlop
Copy link
Contributor

Int16, Int32, Int64, UInt16, UInt32, UInt64 all implement these two methods:

  • IComparable.CompareTo
  • IConvertible.GetTypeCode

@carlossanlop carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases labels Nov 15, 2019
@carlossanlop carlossanlop added this to the November 2019 milestone Nov 15, 2019
@carlossanlop carlossanlop self-assigned this Nov 15, 2019
@carlossanlop carlossanlop requested a review from a team November 15, 2019 20:53
@BillWagner BillWagner self-requested a review November 18, 2019 15:46
@BillWagner BillWagner self-assigned this Nov 18, 2019
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @carlossanlop

I had one question. Once we resolve that, I'll :shipit:

</item>
<item>
<term>Zero</term>
<description>This instance is equal to <paramref name="value" />.</description>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the interface, we've described this as follows:

This current instance occurs in the same position in the sort order as value.

For all the integral types, those two descriptions mean the same. I'll approve this, and you can decide if you want to use the same consistent language. (This comment applies to all pages in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @BillWagner would you mind elaborating a bit more? I'm not sure where you got that description from, so I'm not understanding your suggestion here.

In case it helps: I got all this text from <Member MemberName="CompareTo"> in this same file (line 181), and simply added the extra remark about the EII.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlossanlop The language I quoted above is from the IComparable<T> interface page, in the remarks section.

For all the types in this PR, CompareTo returns 0 if and only if the two instances are equal. That's not true for all types. (Imagine ordering events by date. Two events on the same date would be in the same location in the sort order, but may not be equal.)

For these types, that distinction doesn't matter. I just want to make sure we're consistent and explicit on how we make this decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you mean. I think the descriptions are fine for these int types, they were adapter for their specific behavior.

@carlossanlop carlossanlop merged commit 44250ea into dotnet:master Nov 19, 2019
@carlossanlop carlossanlop deleted the Int_IComparableIConvertible branch November 19, 2019 17:34
@mairaw mairaw removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants