Draft: Migrate from Errors to Exceptions for invalid use of library

[amantinband]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (dcf6122) to head (fcc4099).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #105   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines          423       435   +12     
  Branches        49        54    +5     
=========================================
+ Hits           423       435   +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[feO2x]

Hey Amichai,

I've reviewed your code and created a new PR with suggested changes.

I still have one question: would we allow null to be a valid value for ErrorOr<TValue>? If not, we should change the implementation of

public static implicit operator ErrorOr<TValue>(TValue value)
{
    return new ErrorOr<TValue>(value);
}

to

public static implicit operator ErrorOr<TValue>(TValue value)
{
    if (value is null)
    {
        throw new ArgumentNullException(nameof(value));
    }

    return new ErrorOr<TValue>(value);
}

The reason behind this is the following: users of our library might have <Nullable> turned off. The C# compiler would then no longer create warnings when null is passed as the value for an ErrorOr<TValue> instance (where TValue is resolved to a reference type). This is why we as framework/library devs should check for null (even if we have <Nullable> enabled in our code base).

I'm interested in your opinion on this.

[feO2x]

We could also ask the community if they ever want null to be a valid value.

[amantinband]

Hey Amichai,

I've reviewed your code and created a new PR with suggested changes.

I still have one question: would we allow null to be a valid value for ErrorOr<TValue>? If not, we should change the implementation of

public static implicit operator ErrorOr<TValue>(TValue value)
{
    return new ErrorOr<TValue>(value);
}

to

public static implicit operator ErrorOr<TValue>(TValue value)
{
    if (value is null)
    {
        throw new ArgumentNullException(nameof(value));
    }

    return new ErrorOr<TValue>(value);
}

The reason behind this is the following: users of our library might have <Nullable> turned off. The C# compiler would then no longer create warnings when null is passed as the value for an ErrorOr<TValue> instance (where TValue is resolved to a reference type). This is why we as framework/library devs should check for null (even if we have <Nullable> enabled in our code base).

I'm interested in your opinion on this.

Yeah let's make this more strict as well. In any case this will all be introduced in a new major version. I think your suggestion is a better design.

[feO2x]

Yep, you are right. These are breaking changes, so we need to bump up to version 3.