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

Features/non generic result #89

Merged
merged 6 commits into from
Jun 7, 2022

Conversation

Artem-Romanenia
Copy link
Contributor

@Artem-Romanenia Artem-Romanenia commented May 14, 2022

Attempt to address following issues:

Brief overview:

  • Added class Result : Result<Result> (looks a little tricky but this way no Unit class is needed, inspired by FluentResults)
  • There are a couple of seemingly questionable details which will make sense later:
    • New Result class has a set of static factory methods for creating Result instances almost equivalent to such in Result<T>.
    • Result also has static generic factory methods for creating Result<T> success-instances
    • Result<T> has an implicit cast from Result
  • These quirks allow simplified Result<T> creation in consumer code:
    • Success scenario:
// instead of
return Result<string>.Success("result");
// we can have
return Result.Success("result");
// because Result contains 
public static Result<T> Success<T>(T value)
// that utilizes type inferrence
    • Failure scenarios:
// instead of
return Result<string>.NotFound();
// we can have
return Result.NotFound();
// because Result<T> contains 
public static implicit operator Result<T>(Result result) => new Result<T>(default(T))
{
    Status = result.Status,
    Errors = result.Errors,
    SuccessMessage = result.SuccessMessage,
    ValidationErrors = result.ValidationErrors,
};
// which would be triggered in a typical scenario like
public Result<string> GetStrings(SomeCriteria criteria)
{
    //Do something
    return Result.NotFound(); //This generates Result instance which is casted to Result<string> right away
}
  • Last described scenario has a downside of basically creating two result objects instead of one. But I figured it is acceptable since result object is pretty small anyways, plus failure scenarios should occur much less frequently than success scenarios. Plus, there are still factory methods in Request<T> that will not cause any casting.

Also, there seem to be no braking changes, at least I didn't have to change any existing tests.

If this PR has any potential, but you want me to refactor something, please let me know.

@Artem-Romanenia
Copy link
Contributor Author

@ardalis I'd be happy to hear your feedback on whether any of this makes any sense

@ardalis
Copy link
Owner

ardalis commented Jun 7, 2022

Sorry I've been busy - thanks for pinging me. This looks great based on your comments above.

@ardalis
Copy link
Owner

ardalis commented Jun 7, 2022

...and the build failure is nothing to do with your code...

@ardalis ardalis merged commit eee11fb into ardalis:main Jun 7, 2022
@ardalis
Copy link
Owner

ardalis commented Jun 7, 2022

Looks great, thanks! Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants