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

[Discussion] Revisit AggregateError/ResultCollection #15

Open
Foxtrek64 opened this issue Dec 12, 2023 · 1 comment · May be fixed by #17
Open

[Discussion] Revisit AggregateError/ResultCollection #15

Foxtrek64 opened this issue Dec 12, 2023 · 1 comment · May be fixed by #17

Comments

@Foxtrek64
Copy link
Contributor

Foxtrek64 commented Dec 12, 2023

The current AggregateError type is designed to replicate the AggregateException type, but this results in limited utility and is likely one of the least-used error types in the library.

I would like to replace this type with something with more utility, so I'd like to pose the following questions:
 

  1. What should this type do? Are we wanting a collection of failed results or are we wanting to represent the overall result of bulk operations?
  2. If the latter, do we want separated accessors for successful/unsuccessful/all errors? Do we want to store successful results at all or just counts and a list of the errors.
  3. One of the big things we were looking for with AggregateError was logging. What sort of ToString() (or overloads) would we like to see?
  4. Should this type be a Result or Error in itself, i.e. should it implement IResult/ResultError?
  5. If we handle successful results, how do we handle Result<T> types? Do we provide a generic ResultCollection<TEntity> where all contained results must be Result<TEntity>? Do we keep it non-generic and provide a GetResultOfT<T>() helper?
  6. What additional features would you like to see here?

Here are my thoughts:

  1. Bulk Operation type, such as a ResultCollection.
  2. I'd like separate accessors. For the implementation we can make the type itself implement IEnumerable for the "all results" option and use an ILookup generated at runtime which has a key based on IResult.IsSuccess
  3. I think we should have a format option. Sometimes we just want a success/failure ratio. Sometimes we want more in-depth information.
  4. I don't think this type should be either a result or an error. Implementing ResultError would be inappropriate since this type doesn't represent a failure in itself. Implementing IResult would be inappropriate because we can't have consistent success/failure logic that represents all scenarios.
    • For instance, a user wants to represent the result of deleting a set of files in a folder. Some results are successful (file was deleted), some results failed with a FileNotFoundException. In this case, the consumer wants to treat the FileNotFoundExceptions (as wrapped in ExceptionErrors) as successful.
  5. I think a generic and non-generic option is probably a good idea. Many scenarios will see the stored entity being the same type, but some cases may not.
  6. This would prevent us from using ILookup, allowing functionality like this could be nice:
public ResultCollection ProcessThings(IEnumerable<Thing> things)
{
    foreach (var thing in things)
    {
        yield return thing.DoThingWithResult();
    }
}
public ResultCollection ProcessThingsAsync(IEnumerable<thing> things)
{
    async foreach (var thing in things)
    {
        yield return await thing.DoThingWithResultAsync();
    }
}
@Nihlus
Copy link
Member

Nihlus commented Dec 31, 2023

I like this proposal! We can keep AggregateError for backwards compatibility, but a more flexible collection-oriented result grouping would be quite nice. I often find myself wanting something better than AggregateError, and this proposal definitely looks like the right direction.

@Foxtrek64 Foxtrek64 linked a pull request Jun 4, 2024 that will close this issue
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 a pull request may close this issue.

2 participants