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

Period does not implement operator == #1529

Closed
JvSSD opened this issue May 7, 2020 · 5 comments · Fixed by #1530
Closed

Period does not implement operator == #1529

JvSSD opened this issue May 7, 2020 · 5 comments · Fixed by #1530
Assignees
Milestone

Comments

@JvSSD
Copy link

@JvSSD JvSSD commented May 7, 2020

I happened to create two zero-length periods two different ways to test for equality like so:
var period1 = Period.Zero;
var period2 = PeriodPattern.Roundtrip.Parse("P0D").Value;

Doing period1 == period2 compiles just fine. However, the result was not what I expected. This returns false, not true. On the other hand, doing period1.Equals(period2) does return the expected result of true.

@jskeet jskeet self-assigned this May 7, 2020
@jskeet jskeet added this to the 3.0-consider milestone May 7, 2020
@jskeet
Copy link
Member

@jskeet jskeet commented May 7, 2020

Nicely found. That's a surprise.

Adding the operators is technically a breaking change, but will only make broken code fail, I suspect. It won't break binary compatibility, although it will change the meaning of existing code.

@malcolmr any thoughts?

@jskeet
Copy link
Member

@jskeet jskeet commented May 7, 2020

Labelled as both a bug and an enhancement in that violating the principle of least surprise is arguably a bug :)

@malcolmr
Copy link
Contributor

@malcolmr malcolmr commented May 7, 2020

I think it's reasonable to call this a bugfix and then also call it out in the migration guide.

(Does this also affect equivalence for any dictionary or set-like containers, or do they all use IComparable?)

Is there any way to verify that we haven't missed any operators from any other types?

@jskeet
Copy link
Member

@jskeet jskeet commented May 7, 2020

Set-like things should all be using Equals, which is already implemented.

I could write a test for "classes that implement IEquatable<T> but don't overload ==". There may be some that are deliberate, but we could always put those on an allow-list.

@jskeet
Copy link
Member

@jskeet jskeet commented May 8, 2020

Have now automated this via a unit test. The current type that implement IEquatable<T> but not operator == are:

  • Period
  • TimeZones.Cldr.MapZone
  • ZoneInterval

MapZone is sufficiently complex that I don't think it's appropriate to overload ==, but the other two are okay. (I suspect we mostly have MapZone.Equals for the sake of testing.)

@malcolmr malcolmr removed this from the 3.0-consider milestone May 21, 2020
@malcolmr malcolmr added this to the 3.0 milestone May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants