Conversation
- Refactored WithException, WithHttpStatusCode, and WithHelpLink to be generic and work with both Result and Result<T>. - Added WithBag extension and BagItems property for attaching and accessing metadata on Result objects. - Introduced ObjectConversion utility with TryGetSuccessValue, ToSuccessResult, and ToFailureResult methods for easier value/result conversions. - Improved XML documentation and formatting throughout the codebase. - Minor .csproj cleanup for packaging and documentation.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Result object extensions to use generic type constraints, adds a metadata/property bag feature to Result objects, and introduces utility methods for converting values to Result objects.
Changes:
- Consolidated separate Result and Result extension methods (WithException, WithHttpStatusCode, WithHelpLink) into single generic methods with
where T : Resultconstraints - Added metadata storage capability through WithBag extension method and BagItems property
- Introduced ObjectConversion utility class with TryGetSuccessValue, ToSuccessResult, and ToFailureResult helper methods
- Enhanced XML documentation throughout
- Minor formatting cleanup in .csproj file
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ResultExtensions.cs | Refactored extension methods to use generic type constraints, eliminating duplicate code; added new WithBag method for metadata storage |
| Result.cs | Added internal Metadata dictionary and public BagItems property for metadata access; updated XML doc references |
| Conversion/ObjectConversion.cs | New utility class providing conversion helpers between values and Result objects |
| IResultValue.cs | Added XML documentation to the interface |
| ResultOfT.cs | Removed unused System.Diagnostics.CodeAnalysis import |
| Arcturus.ResultObjects.csproj | Minor formatting cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <returns>A Result<T> instance representing a successful operation with the specified value.</returns> | ||
| public static Result<T> ToSuccessResult<T>(this T value) | ||
| { | ||
| return Result<T>.Success<T>(value); |
There was a problem hiding this comment.
The type parameter specification '' is redundant in this static method call. The type can be inferred from the return type. This should be simplified to 'Result.Success(value)'.
| /// <returns>A failed result of type <typeparamref name="T"/> containing the specified fault.</returns> | ||
| public static Result<T> ToFailureResult<T>(this T value, Fault? fault) | ||
| { | ||
| return Result<T>.Failure<T>(fault); |
There was a problem hiding this comment.
The type parameter specification '' is redundant in this static method call. The type can be inferred from the return type. This should be simplified to 'Result.Failure(fault)'.
| /// <returns>A failed result of type T containing the specified error code and message.</returns> | ||
| public static Result<T> ToFailureResult<T>(this T value, string? code, string message) | ||
| { | ||
| return Result<T>.Failure<T>(code, message); |
There was a problem hiding this comment.
The type parameter specification '' is redundant in this static method call. The type can be inferred from the return type. This should be simplified to 'Result.Failure(code, message)'.
| /// Creates a failed result containing the specified fault for the given value. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the value associated with the result.</typeparam> | ||
| /// <param name="value">The value to associate with the failed result. This value is not used in the failure case but is required for | ||
| /// extension method syntax.</param> | ||
| /// <param name="fault">The fault information to include in the failed result. Can be null to indicate an unspecified failure.</param> | ||
| /// <returns>A failed result of type <typeparamref name="T"/> containing the specified fault.</returns> | ||
| public static Result<T> ToFailureResult<T>(this T value, Fault? fault) | ||
| { | ||
| return Result<T>.Failure<T>(fault); | ||
| } | ||
| /// <summary> | ||
| /// Creates a failed result with the specified error code and message. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the value associated with the result.</typeparam> | ||
| /// <param name="value">The value to associate with the failed result. This value is not used in the failure case but is required for | ||
| /// extension method syntax.</param> | ||
| /// <param name="code">The error code that identifies the reason for failure. Can be null if no code is applicable.</param> | ||
| /// <param name="message">A message describing the failure. Cannot be null.</param> | ||
| /// <returns>A failed result of type T containing the specified error code and message.</returns> | ||
| public static Result<T> ToFailureResult<T>(this T value, string? code, string message) |
There was a problem hiding this comment.
The 'value' parameter in these ToFailureResult extension methods is misleading and unused. The methods create a failure result that doesn't use the input value at all. This API design could confuse users who might expect the value to be stored or used somehow.
Consider making these static methods on a utility class instead of extension methods, or clarifying in the documentation that the value parameter is ignored and exists only to enable extension method syntax. Alternatively, consider whether these extension methods provide sufficient value over calling Result.Failure directly.
| /// Creates a failed result containing the specified fault for the given value. | |
| /// </summary> | |
| /// <typeparam name="T">The type of the value associated with the result.</typeparam> | |
| /// <param name="value">The value to associate with the failed result. This value is not used in the failure case but is required for | |
| /// extension method syntax.</param> | |
| /// <param name="fault">The fault information to include in the failed result. Can be null to indicate an unspecified failure.</param> | |
| /// <returns>A failed result of type <typeparamref name="T"/> containing the specified fault.</returns> | |
| public static Result<T> ToFailureResult<T>(this T value, Fault? fault) | |
| { | |
| return Result<T>.Failure<T>(fault); | |
| } | |
| /// <summary> | |
| /// Creates a failed result with the specified error code and message. | |
| /// </summary> | |
| /// <typeparam name="T">The type of the value associated with the result.</typeparam> | |
| /// <param name="value">The value to associate with the failed result. This value is not used in the failure case but is required for | |
| /// extension method syntax.</param> | |
| /// <param name="code">The error code that identifies the reason for failure. Can be null if no code is applicable.</param> | |
| /// <param name="message">A message describing the failure. Cannot be null.</param> | |
| /// <returns>A failed result of type T containing the specified error code and message.</returns> | |
| public static Result<T> ToFailureResult<T>(this T value, string? code, string message) | |
| /// Creates a failed result containing the specified fault. | |
| /// </summary> | |
| /// <typeparam name="T">The type parameter of the result.</typeparam> | |
| /// <param name="fault">The fault information to include in the failed result. Can be null to indicate an unspecified failure.</param> | |
| /// <returns>A failed result of type <typeparamref name="T"/> containing the specified fault.</returns> | |
| public static Result<T> ToFailureResult<T>(Fault? fault) | |
| { | |
| return Result<T>.Failure<T>(fault); | |
| } | |
| /// <summary> | |
| /// Creates a failed result with the specified error code and message. | |
| /// </summary> | |
| /// <typeparam name="T">The type parameter of the result.</typeparam> | |
| /// <param name="code">The error code that identifies the reason for failure. Can be null if no code is applicable.</param> | |
| /// <param name="message">A message describing the failure. Cannot be null.</param> | |
| /// <returns>A failed result of type <typeparamref name="T"/> containing the specified error code and message.</returns> | |
| public static Result<T> ToFailureResult<T>(string? code, string message) |
| { | ||
| result.HelpLink = uri; | ||
| result.Metadata ??= []; | ||
| result.Metadata.Add(key, value); |
There was a problem hiding this comment.
The WithBag method doesn't check if the key already exists in the metadata dictionary before adding. This will throw an ArgumentException if the same key is added twice. Consider using dictionary indexer syntax (result.Metadata[key] = value) instead of Add, which will overwrite existing values, or document that duplicate keys will throw an exception.
| result.Metadata.Add(key, value); | |
| result.Metadata[key] = value; |
| /// <summary> | ||
| /// Gets additional metadata items. Use <see cref="ResultExtensions.WithBag{T}(T, string, object?)"/>. | ||
| /// </summary> | ||
| public IReadOnlyDictionary<string, object?>? BagItems => Metadata; |
There was a problem hiding this comment.
The naming is inconsistent between the method name WithBag and the property name BagItems. For consistency and clarity, consider either:
- Renaming WithBag to WithBagItem (matching BagItems), or
- Renaming to a more standard metadata terminology like WithMetadata/MetadataItems or WithProperty/Properties
The term "bag" is somewhat informal and may not clearly communicate that this is for metadata storage. Compare with EventContext in the codebase which uses "Items" for its property bag.
| public IReadOnlyDictionary<string, object?>? BagItems => Metadata; | |
| public IReadOnlyDictionary<string, object?>? MetadataItems => Metadata; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary & Motivation
A brief description of the changes in this pull request explaining why these changes are necessary. Please delete this paragraph.
Checklist