k2workflow / clay Goto Github PK

• Ambiguous constructors (byte[], span, etc)
• Null handling
• Unit coverage - there were units at some stage, where are they now
• Ensure Fnv(null) returns a consistent (chosen) value
• Rationalize multiple unused overloads
• Fix class name, and follow method naming convention from System.HashCode

Some of the serialization classes in OpenApi are large and complex.
Split these into partial class instead

eg OasSerializer.Serialize.cs

See https://developercommunity.visualstudio.com/content/problem/133059/vs2017-code-analysis-not-working.html

We use reflection elsewhere to get the number of members in an Enum.
Optimize this to use expressions and/or cached values.
See #90

Prefix all classes with Oas*

Many of the null checks use XOR.
This is bad for branch prediction (@jcdickinson)
Maybe use this pattern: if (x is null) return y is null;

https://github.com/k2workflow/Clay/blob/master/LICENSE

Please use the following convention for equality.
This pattern occurs in many places in OpenApi.

public override bool Equals(object obj)
    => obj is ApiKeySecurityScheme key // short-circuits if object is unexpected type
    && Equals(key);

This pattern incurs a similar cast, but does not short-circuit the nested call to Equals(T):

... => Equals(obj as ApiKeySecurityScheme)

Create a class that allows converting between enums and integers.

We need a SpanEquals extension that can compare two ReadOnlySpan<T> in the same optimized manner as ListEquals, etc

For example, does not check for this == null:

        public virtual bool Equals(Contact other)
        {
            if (other is null) return false; // Existing convention is that equal(null, null) = true
            if (ReferenceEquals(this, other)) return true; // OK

            if (!StringComparer.Ordinal.Equals(Name, other.Name)) return false; // throws if other==null

Furthermore, the following checks should be included above (unless it's by-design to have different semantics here):

        public static bool operator ==(Contact contact1, Contact contact2)
        {
            if (contact1 is null && contact2 is null) return true;
            if (contact1 is null || contact2 is null) return false;
            return contact1.Equals((object)contact2);
        }

Rather, use a pattern like this:

        public bool Equals(ReadOnlyJsonObject other) => Equals(this, other);

        public static bool operator ==(ReadOnlyJsonObject x, ReadOnlyJsonObject y) => Equals(x, y);

        // Logic is in one method, not spread across two
        private static bool Equals(ReadOnlyJsonObject x, ReadOnlyJsonObject y)
        {
            if (x is null) return y is null; // (null, null) or (null, y)
            if (y is null) return false; // (x, null)
            if (ReferenceEquals(x, y)) return true; // (x, x)

            if (!x._json.NullableJsonEquals(y._json)) return false;

            return true;
        }

• SourceCode.Json needs a DecimalValidator to parallel the ones for double and int64
• Per framework design guidelines, LongValidator should be called Int64Validator

• Json serializers for OpenApi
• Json deserializers for OpenApi
• Factor into partial classes (per #108 )
• Units not working (#203)
• Extra Units

• Use Directory.Build.props for common project <properties> such as License, etc

• Use editorconfig for editor conventions

• NetCoreApp2.0 no longer requires System.ValueTuple Nuget

Right now it only accepts IReadOnlyDictionary

StringComparers that validate values according to spec need to be created.

All string dictionaries should use these comparers.

Per similar PR in Chasm

Several extension methods are called NullableEquals since they all deal properly with null values.
However the name is ambiguous both in terms of callsite intent and callsite overloads.
It is hard for a caller to choose which method they actually want and/or which overload has been resolved.

• Rather name each method differently - eg ListEquals, DictionaryEquals and so on
• Permit intermixed parameters: IReadOnly/List, IReadOnly/Collection, IEnumerable, ISet
• Remove bool sequential parameter: Callers should use SetEquals for non-sequential comparisons

We used to have such a benchmark, it showed a significant gain.
Not sure why it didn't get pushed originally, but either way we need another one.

NumberKind = 0 does not innately represent anything useful.
Maybe find a better set of Flag values & ordinals

Because unlike Substring it's used primarily for display purposes so should not return broken text

Add a method that waits for a file lock.

• We have lots of complexity in ReadOnly/ListBufferComparer and EnumerableComparer. But they equality checks are already covered in Clay.Collections.NullableEquals.
• HashCode.Fnv should not throw for null or empty inputs

Some refs are being serialized as straight strings instead of e.g. { '$ref': '#/components/schemas/foo' }

Clay.Number is a union type, useful for scenarios (such as System.Json) where numeric fidelity is important but the system may return 1 of several different types of numerics.

In specific use cases we need to support Decimal as well as the int and real types already supported.
This will bloat the size of the union unfortunately.

#12 removes functionality that arises due to packages being incompatible with .Net Core 2.0. This functionality should be restored.

enum Kind { Integer, Float, Null }

This can be used to disambiguate the general class of types encapsulated in the union - for example, Integer covers all 8 integer subtypes.

A Boolean would not suffice since Null is neither (a Nullable may work but is ugly to use)

Defined enum values should be checked during strict/checked conversion.

2 of the Units are failing with a NullRef exception, but only on AppVeyor.
Local execution works fine.

• Useful for BCL classes that require it, eg Array.Sort(..., Comparison)
• Units

PR #63 added support for SpanEquals<T>.
However, from the call site this is not straightforward to use since the following idiom needs to be in place:

  // Memory<T>.Span throws if IsEmpty == true
  if (x._nodes.Length != y._nodes.Length) return false;
  if (x._nodes.IsEmpty) return true;;

  return x._nodes.Span.SpanEquals(y._nodes.Span, true);

This idiom is non-obvious.
I suggest we replace these methods with MemoryEquals<T> that operates on ReadOnlyMemory<T>

See alternatives here: #122
(Pushed representative changes to OasContact)

To compare instances of Number

Rename this to NullableSetEquals or similar, since SetEquals already exists on ISet

As of latest VS, the compiler does not permit generics using Span as T

There are many redundant and unused functions in Clay that in retrospect are solution-specific so don't belong in a general library.
Remove them; we can always revive if/when they are needed.

This code fails with ArgumentNullException:
BufferComparer.Default.Equals(Data, other.Data)
where Data is byte[] and ==null

Once we have the email, we need to create/update the 3rd party CI accounts:

• AppVeyor
• CodeCov
• CodeFactor

Number already has the intrinsic ability to store null
But some callers instantiate an instance of Nullable<Number>

We either need to remove the intrinsic null, or change the callers.

For example, this seems ok:

var foo = new Number(...);
var valid = foo.HasValue;

But this is a problem:

var foo = new Nullable<Number>(...);
var valid1 = foo.HasValue;
var valid2 = foo.Value.HasValue; // Nooooo!