Make simple types comparable and equatable #487

ThomasBarnekow · 2018-04-20T09:52:10Z

In conjunction with #485, this commit introduces two base classes,
OpenXmlComparableReferenceType and OpenXmlComparableValueType, which
implement the IComparable, IComparable<T>, and IEquatable<T>
interfaces. With the exception of EnumValue and ListValue, all children
or descendants of OpenXmlSimpleType are changed to become subclasses of
one of those two base classes.

This commit further adds unit tests for the new behavior. The core
tests are contained in the OpenXmlComparableReferenceTypeTests and
OpenXmlComparableValueTypeTests base classes. For each concrete simple
type, subclasses of those base classes create the concrete values to be
used in the generic unit tests.

Lastly, this commit fixes existing unit tests within the SimpleTypeTest
and OpenXmlSimpleValueTest classes, because those unit tests asserted
that two instances with same value but constructed separately are NOT
equal.

Note that the implementation is not yet complete as it lacks comments
for some public methods, for example. We might also have to add further
unit tests.

In conjunction with dotnet#485, this commit introduces two base classes, OpenXmlComparableReferenceType and OpenXmlComparableValueType, which implement the IComparable, IComparable<T>, and IEquatable<T> interfaces. With the exception of EnumValue and ListValue, all children or descendants of OpenXmlSimpleType are changed to become subclasses of one of those two base classes. This commit further adds unit tests for the new behavior. The core tests are contained in the OpenXmlComparableReferenceTypeTests and OpenXmlComparableValueTypeTests base classes. For each concrete simple type, subclasses of those base classes create the concrete values to be used in the generic unit tests. Lastly, this commit fixes existing unit tests within the SimpleTypeTest and OpenXmlSimpleValueTest classes, because those unit tests asserted that two instances with same value but constructed separately are NOT equal. Note that the implementation is not yet complete as it lacks comments for some public methods, for example. We might also have to add further unit tests.

This commit removes the IConvertible type constraint on the OpenXmlComparableValueType class. While working just fine in a local VS2017 build, this constraint led to build errors on the build server.

This commit fixes the DecimalValueTest, DoubleValueTest, and SingleValueTest methods of the OpenXmlSimpleValueTest class. Those methods failed on machines using a locale (or culture settings) that are not compatible with the en-US locale (or invariant culture). Fixes dotnet#488.

ThomasBarnekow · 2018-04-20T11:15:45Z

@twsouthwick I've seen that the build failed because what are mere warnings in a local build is taken as an error on the build server. For example, the warning (or error on the build server) is that certain classes should implement Equals because they are implementing IComparable. However, they don't need to implement Equals, because all they would do is delegate that to the base class, which renders that implementation unnecessary.

I did not want to disable warnings etc., because I wanted to discuss this whole approach with you anyhow. But note that it builds just fine on my machine and all unit tests pass.

twsouthwick · 2018-04-20T15:16:13Z

src/DocumentFormat.OpenXml/SimpleTypes/EnumValue.cs

@@ -10,15 +10,14 @@ namespace DocumentFormat.OpenXml
    /// Represents the enum value for attributes.
    /// </summary>
    /// <typeparam name="T">Every enum value must be an enum with the EnumStringValueAttribute object.</typeparam>
-    [DebuggerDisplay("{InnerText}")]
+    [DebuggerDisplay("{" + nameof(InnerText) + "}")]
    public class EnumValue<T> : OpenXmlSimpleValue<T>
        where T : struct


For v3.0, we can add a constraint for Enum (as of C# 7.3) :)

@twsouthwick Do you want me to do anything here? ReSharper suggested this change and I've used nameof in my code as much as possible to avoid issues as names change (which might not be the case here).

Yeah, I saw that change as I quickly glanced through. I like the use of nameof as much as possible and see the value here. However, that name won't be changing, so I'd probably not make the change myself as it takes a bit more time to grok. I'll leave that up to you, though

twsouthwick · 2018-04-20T15:17:01Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableReferenceType.cs

+        /// <summary>
+        /// Creates a new instance of <see cref="OpenXmlComparableReferenceType{T}"/>.
+        /// </summary>
+        protected OpenXmlComparableReferenceType()


Can you make these constructors private protected? I'd like to limit visibility of new types/methods as much as possible unless we have a reason to make them public

Sure, can do.

What about the existing constructors of OpenXmlSimpleType and OpenXmlSimpleValue<T>, which are all protected? Should we consider making those private protected in v3.0?

twsouthwick · 2018-04-20T15:18:36Z

Thanks for the submission! I'll take a look in the next couple days. As far as the warnings go, sounds like that may be an issue with the StyleCop.Analyzers. You can disable the warnings for each class with an explanation. We should file an issue at their repo for that because it does seem unnecessary

twsouthwick · 2018-04-20T15:19:52Z

My comment was wrong - this isn't StyleCop errors, the error id is CA1036 which is for Code Analysis

twsouthwick · 2018-04-20T15:48:12Z

I've created an issue on their repo that you can reference when you disable the code with a pragma: dotnet/roslyn-analyzers#1671

This commit disables CA1036 for subclasses of the generic base classes OpenXmlComparableReferenceType and OpenXmlComparableValueType to avoid the unnecessary warning that those subclasses should implement Equals. See dotnet/roslyn-analyzers#1671 for details. This commit also adds file headers where those were missing.

This commit adds missing file headers and moves the DoubleValueTests class into its own file.

twsouthwick · 2018-04-22T00:36:59Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableReferenceType.cs

+        /// <summary>
+        /// Gets or sets the value of this instance.
+        /// </summary>
+        public abstract T Value { get; set; }


Does this need to be abstract? Or can it just be virtual (or even non-virtual)? It would nice to follow the pattern of OpenXmlSimpleValue<T>

I thought about that as well. Currently, only three subclasses inherit from OpenXmlComparableReferenceType<T>:

Base64BinaryValue,

HexBinaryValue, and

StringValue.

All of those basically delegate to OpenXmlSimpleType.TextValue. Therefore, I thought about two options:

turn OpenXmlComparableReferenceType<T> into something like OpenXmlStringType and provide a concrete implementation of the Value property, or

leave OpenXmlComparableReferenceType<T> generic to enable reuse for non-string types.

For the second option, which I chose for this implementation, I thought it was more elegant to just keep the that property abstract. However, we could also go for Option 1.

Ah - I hadn't realized it was a generic type. I like the current implementation better so it's more reusable.

twsouthwick · 2018-04-22T00:38:30Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableReferenceType.cs

+                return CompareTo(other);
+            }
+
+            throw new ArgumentException();


Should there be a message here?

Easy answer: yes.

haha now the hard question is figuring out what to put :)

Here's my current thinking (and I am very much open for suggestions):

public int CompareTo(object obj) { switch (obj) { case null: return 1; case OpenXmlComparableReferenceType<T> other: return CompareTo(other); } throw new ArgumentException("Incompatible type: " + obj.GetType().FullName); }

I also used a switch statement with pattern matching.

I'm fine with that - just stick it in the resource file

twsouthwick · 2018-04-22T00:39:42Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableReferenceType.cs

+                return other.Value == null;
+            }
+
+            return Value.Equals(other.Value);


Can this be collapsed into Equals(Value, other?.Value)?

That depends on whether a non-null instance with a null Value property should be equal to a null instance.

For example, I could have two non-null StringValue instances, with Value being null for both of them. Those two should be considered equal. However, when I take any of those, I'd say that stringValue.Equals(null) should be false. In this case, the shortened Equals method could look like this:

public bool Equals(OpenXmlComparableReferenceType<T> other) { return !(other is null) && Equals(Value, other.Value); }

Hmm I still think that Equals(Value, other?.Value) would work, but I guess I'm not sure off the top of my head what Equals(null,null) is. Do you have unit tests covering the cases you bring up? If so you can just try it out. I'll leave this up to you - no need to rathole into it as the current implementation works

Equals(null, null) should be true.

twsouthwick · 2018-04-22T00:42:01Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableValueType.cs

+    /// Represents a comparable and equatable value for simple value types (Int32, UInt32, Byte, struct, etc).
+    /// </summary>
+    /// <typeparam name="T">The type of the value.</typeparam>
+    public abstract class OpenXmlComparableValueType<T> : OpenXmlSimpleValue<T>,


What if OpenXmlComparableValueType<T> : OpenXmlComparableReferenceType<T?>? This could probably consolidate a bit of code.

Actually, that's probably a breaking change (it would change the inheritance chain of some of the current values)

I've also thought about doing more cleanup. However, I've tried to limit the changes to the inheritance chain to the minimum to not introduce a breaking change.

Yeah that's good for now. We can get the functionality in, and then mark it for clean up with an issue opened against the v3.0 milestone

twsouthwick

I added a couple of comments about naming. Other than this, it all looks good. We can just work through any comments and then get it merged in

twsouthwick · 2018-04-22T16:51:34Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableReferenceType.cs

+    /// Represents a comparable and equatable reference.
+    /// </summary>
+    /// <typeparam name="T">The type of the value.</typeparam>
+    public abstract class OpenXmlComparableReferenceType<T> : OpenXmlSimpleType,


The name feels a little weird. Can we follow the previous pattern and go with OpenXmlComparableSimpleType?

Yes, it is a little weird, so I am open for better names. But let me explain the thinking behind the current name.

On the master branch, we have OpenXmlSimpleType and OpenXmlSimpleValue<T> where the latter derives from the former. The subclasses representing value types (e.g., DecimalValue) and EnumValue<T> derive from OpenXmlSimpleValue<T>. The string-valued (reference) types (e.g., StringValue, HexBinaryValue) and ListValue<T> derive from OpenXmlSimpleType.

Now, to not introduce a breaking change, I needed to introduce:

one subclass of OpenXmlSimpleValue<T> (and therefore a grandchild of OpenXmlSimpleType) for all those comparable and equatable value types, hence the name OpenXmlComparableValueType<T>; and

one subclass of OpenXmlSimpleType that would become the superclass of the "reference types" StringValue, HexBinaryValue, and Base64BinaryValue.

For the second, the best name I could come up with was OpenXmlComparableReferenceType, because that was in contrast to OpenXmlComparableValueType, which was kind of given by its parent class OpenXmlSimpleValue.

Now, since both of those new, abstract base classes are "comparable simple types", we should not call just one of them OpenXmlComparableSimpleType.

Having said that, I am totally not wedded to the current names. If we find something that is logical, I am fine with that.

Yeah, naming is hard 😆 I see your thought process and I think the issue I'm having with the name is "ValueType"/"ReferenceType" doesn't imply a kind of OpenXmlSimpleType. I want to make sure it's easy to see the relationship in the names (if possible). How about OpenXmlComparableSimpleValue<T> and OpenXmlComparableSimpleReference<T>. Not sure if they should be suffixed with "Type", but I think what I'm trying to get is a name that is obviously a subclass of OpenXmlSimpleType.

OpenXmlComparableSimpleValue<T> and OpenXmlComparableSimpleReference<T> would be fine for me. I think I thought about the SimpleReference as well, but wasn't sure whether that introduces an unwanted connotation. But if you are fine with those, we can certainly also use those two.

And if you like OpenXmlComparableSimpleType<T> better, we could also go with that. As noted in my other comment, my only thought would be whether OpenXmlComparableSimpleValue<T> should derive from OpenXmlComparableSimpleType<T>, if possible.

I like the first two. I agree they're not ideal, but it's a good compromise to keep naming consistent.

If we can get OpenXmlComparableSimpleValue<T> to derive from OpenXmlComparableSimpleType<T> that would be awesome, but I think it would break the hierarchy. Note the PR has a package-compat-analyzer check I wrote to validate that we don't break binary compatibility, so if you're not sure, you can submit an update and see what that analyzer says.

twsouthwick · 2018-04-22T16:52:15Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableValueType.cs

+    /// Represents a comparable and equatable value for simple value types (Int32, UInt32, Byte, struct, etc).
+    /// </summary>
+    /// <typeparam name="T">The type of the value.</typeparam>
+    public abstract class OpenXmlComparableValueType<T> : OpenXmlSimpleValue<T>,


As with the previous comment for the reference type, I think OpenXmlComparableSimpleValue would fit better with the current naming

Taking your two comments together:

OpenXmlComparableReferenceType<T> becomes OpenXmlComparableSimpleType<T>

OpenXmlComparableValueType<T> becomes OpenXmlComparableSimpleValue<T>

I am totally fine with (2). I am just thinking that you'd expect OpenXmlComparableSimpleValue<T> to derive from OpenXmlComparableSimpleType<T> just as OpenXmlSimpleValue<T> derives from OpenXmlSimpleType.

Can we do that without breaking anything?

The constraints on T are different for both classes, because one wants T to be a class while the other wants T to be a struct. Not sure what happened if we left out the class constraint and only added struct in the child (for the value types).

If we were to derive such as OpenXmlComparableSimpleValue<T> : OpenXmlComparableSimpleType<T?> would that work? I still think we'd end up breaking the inheritance chain which we'd need to do for v3.0. If you agree, can you open an issue to track making this change in v3.0?

This commit introduces enhancements as discussed on GitHub and adds further unit tests.

ThomasBarnekow · 2018-04-22T20:35:07Z

@twsouthwick I've pushed a commit with the changes we've discussed.

BTW, I am getting messages that Travis CI builds are failing.

twsouthwick · 2018-04-22T21:36:43Z

What Travis CI failures are you referring to? I only know of AppVeyor

twsouthwick

Looks great! The last thing to fix is to remove the comments I pointed out in the tests discussing what will break and what won't. After this change, that will become the current behavior

twsouthwick · 2018-04-22T21:41:26Z

test/DocumentFormat.OpenXml.Tests/OpenXmlDomTest/OpenXmlSimpleValueTest.cs

@@ -84,7 +85,13 @@ public void BooleanValueTest()
            Log.Comment("Verifying reference type behavior...");
            objA = new BooleanValue(validValue);
            objB = new BooleanValue(validValue);
-            Log.VerifyFalse(object.Equals(objA, objB), "Two instances with same value but constructed separated are Equal.");
+
+            // This is the previous assertion, which fails due to the proposed change:


Can you remove these comments and update it to use Assert.Equal instead - the Log file system is something I'd like to move away from as XUnit provides most of the testing stuff (and helpful diagnostics) we need

I can remove the comments that I added. However, those Log.VerifyFalse() calls are from the original author of those unit tests and there are a lot of those ...

Sounds good. I'm not saying remove all the Log.Verify[...] calls, just switch to XUnit for any new code :) I've been chipping away at the removal of the old Log infrastructure for a while now... I definitely don't expect that in this PR :)

ThomasBarnekow · 2018-04-22T22:27:16Z

The Travis CI failures came from an outdated build that I had once configured for my repo when we originally set up build servers for the Open XML SDK.

ThomasBarnekow · 2018-04-22T22:29:34Z

src/DocumentFormat.OpenXml/SimpleTypes/OpenXmlComparableSimpleReference.cs

+        /// <param name="right"></param>
+        /// <returns></returns>
+        public static bool operator <(OpenXmlComparableSimpleReference<T> left, OpenXmlComparableSimpleReference<T> right)
+        {


What about the comments on those operators (==, !=, <, <=, >, >=)? I have not completed them but don't know whether we really need them.

hmmm for completeness, either fill them in (probably some boiler plate comments), or just remove them (unless if stylecop complains - I can't remember if I've turned those on yet)

I added those empty comments because Style Cop did complain. I will steal something elsewhere.

twsouthwick · 2018-04-23T00:13:47Z

Ah, yes, if you set up TravisCI for an old build system, it will most definitely break. The new project uses new csproj and requires VS 2017 Update 3 (or 5... can't remember). Right now we're using AppVeyor, but it would be nice to expand that out to test on Mono and multiple locales (and potentially Xamarin and .NET Native), but that would be for a different (or set of) PRs

This commit removes comments added to existing unit tests to describe the change in behavior. Further, it replaces Log.VerifyTrue() with Assert.True() for the parts of the unit tests that establish equality.

This commit adds missing comments for the operators ==, !=, <, <=, >, and >=.

twsouthwick

LGTM! Once the build finishes I'll merge it

twsouthwick · 2018-04-24T02:54:27Z

The test that failed is one I just added... I'll merge this and then figure out what's causing those failures

ThomasBarnekow added 3 commits April 20, 2018 11:48

Remove IConvertible type constraint

c9c5b13

This commit removes the IConvertible type constraint on the OpenXmlComparableValueType class. While working just fine in a local VS2017 build, this constraint led to build errors on the build server.

twsouthwick reviewed Apr 20, 2018

View reviewed changes

ThomasBarnekow added 3 commits April 21, 2018 22:30

Add missing file headers

d614d0f

This commit adds missing file headers and moves the DoubleValueTests class into its own file.

Make constructors private protected

9d85097

twsouthwick reviewed Apr 22, 2018

View reviewed changes

Enhance based on discussion

6cde609

This commit introduces enhancements as discussed on GitHub and adds further unit tests.

twsouthwick reviewed Apr 22, 2018

View reviewed changes

ThomasBarnekow commented Apr 22, 2018

View reviewed changes

ThomasBarnekow and others added 3 commits April 23, 2018 16:52

Remove comments and use Assert

209afde

This commit removes comments added to existing unit tests to describe the change in behavior. Further, it replaces Log.VerifyTrue() with Assert.True() for the parts of the unit tests that establish equality.

Add missing comments for operators

b328b1a

This commit adds missing comments for the operators ==, !=, <, <=, >, and >=.

Merge branch 'master' into feature/simple-type-equality

3a2ee17

twsouthwick approved these changes Apr 24, 2018

View reviewed changes

twsouthwick merged commit 3cc2657 into dotnet:master Apr 24, 2018

ThomasBarnekow deleted the feature/simple-type-equality branch September 7, 2019 08:30

Make simple types comparable and equatable #487

Make simple types comparable and equatable #487

Conversation

ThomasBarnekow commented Apr 20, 2018 • edited

ThomasBarnekow commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twsouthwick commented Apr 20, 2018

twsouthwick commented Apr 20, 2018

twsouthwick commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twsouthwick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBarnekow Apr 22, 2018 • edited

Choose a reason for hiding this comment

twsouthwick Apr 22, 2018 • edited

Choose a reason for hiding this comment

ThomasBarnekow commented Apr 22, 2018

twsouthwick commented Apr 22, 2018

twsouthwick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBarnekow commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twsouthwick commented Apr 23, 2018

twsouthwick left a comment

Choose a reason for hiding this comment

twsouthwick commented Apr 24, 2018

ThomasBarnekow commented Apr 20, 2018 •

edited

ThomasBarnekow Apr 22, 2018 •

edited

twsouthwick Apr 22, 2018 •

edited