Additional Result and Maybe extensions#357
Conversation
…ueOrDefault, Maybe.Bind/Or/Where/Tap, TryFirst/TryLast, Choose Result extensions: - Check: validate without losing pipeline value (sync + full async matrix) - BindZip: sequential tuple accumulation with T4 template (2→9 tuples) - MapIf: conditional pure transformation (bool + predicate overloads) - EnsureNotNull: null-guard + type narrowing for class and struct - GetValueOrDefault: terminal extraction (value, Func<T>, Func<Error,T>) Maybe instance methods: - Bind: flatMap for Maybe chains - Or: fallback value/Maybe (4 overloads) - Where: predicate filter - Tap: side effect - GetValueOrDefault(Func<T>): lazy default Collection helpers: - TryFirst/TryLast: safe IEnumerable → Maybe bridge - Choose: filter+unwrap IEnumerable<Maybe<T>> 108 new tests, all passing. Zero build warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update trellis-api-reference.md, Trellis.Results README, and maybe-type.md docfx article with Check, BindZip, MapIf, EnsureNotNull, GetValueOrDefault, Maybe.Bind/Or/Where/Tap, TryFirst/TryLast, and Choose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
==========================================
- Coverage 85.32% 85.02% -0.31%
==========================================
Files 205 223 +18
Lines 7174 7457 +283
Branches 1398 1447 +49
==========================================
+ Hits 6121 6340 +219
- Misses 656 718 +62
- Partials 397 399 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add missing resultTask null checks in BindZipAsync overloads - Add missing ValueTask/ValueTask tuple chaining overload to T4 template - Remove unnecessary notnull constraint from Choose<T, TResult> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds new functional helpers to Trellis’ ROP surface area by expanding Result<T> pipelines (validation, conditional mapping, tuple accumulation, terminal extraction) and enhancing Maybe<T> ergonomics (bind/filter/fallback/tap) plus safe collection-to-Maybe helpers, with accompanying tests and documentation updates.
Changes:
- Added new
Result<T>extensions:Check,BindZip(T4 template),MapIf,EnsureNotNull, andGetValueOrDefault. - Added new
Maybe<T>instance methods (Bind,Or,Where,Tap, lazyGetValueOrDefault) and collection helpers (TryFirst/TryLast,Choose). - Updated docs (API reference, Maybe article, package README) and added/extended tests for the new APIs.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| trellis-api-reference.md | Documents newly added Result/Maybe APIs and collection helpers. |
| docs/docfx_project/articles/maybe-type.md | Updates Maybe guide with new methods and examples. |
| Trellis.Results/README.md | Adds README sections describing new extensions and collection helpers. |
| Trellis.Results/src/Trellis.Results.csproj | Registers new T4 template output and template file metadata. |
| Trellis.Results/src/Result/Extensions/MapIf.cs | Implements Result<T>.MapIf(...) overloads. |
| Trellis.Results/src/Result/Extensions/GetValueOrDefault.cs | Implements terminal Result<T>.GetValueOrDefault(...) overloads. |
| Trellis.Results/src/Result/Extensions/Ensure.cs | Adds EnsureNotNull narrowing for Result<T?> → Result<T> (class/struct). |
| Trellis.Results/src/Result/Extensions/Check.cs | Implements sync Check extensions. |
| Trellis.Results/src/Result/Extensions/Check.Task.cs | Implements Task “both async” CheckAsync. |
| Trellis.Results/src/Result/Extensions/Check.Task.Left.cs | Implements Task “left async” CheckAsync. |
| Trellis.Results/src/Result/Extensions/Check.Task.Right.cs | Implements Task “right async” CheckAsync. |
| Trellis.Results/src/Result/Extensions/Check.ValueTask.cs | Implements ValueTask “both async” CheckAsync. |
| Trellis.Results/src/Result/Extensions/Check.ValueTask.Left.cs | Implements ValueTask “left async” CheckAsync. |
| Trellis.Results/src/Result/Extensions/Check.ValueTask.Right.cs | Implements ValueTask “right async” CheckAsync. |
| Trellis.Results/src/Result/Extensions/BindZip.cs | Implements base BindZip (scalar → 2-tuple). |
| Trellis.Results/src/Result/Extensions/BindZip.Task.cs | Implements Task async BindZipAsync (scalar → 2-tuple, both async). |
| Trellis.Results/src/Result/Extensions/BindZip.Task.Left.cs | Implements Task async BindZipAsync (scalar → 2-tuple, left async). |
| Trellis.Results/src/Result/Extensions/BindZipTs.g.tt | T4 template generating tuple-growing BindZip/BindZipAsync overloads up to 9-tuple. |
| Trellis.Results/src/Maybe/Maybe{T}.cs | Adds new Maybe<T> instance methods and lazy default factory overload. |
| Trellis.Results/src/Maybe/Extensions/TryFirst.cs | Adds TryFirst/TryLast enumerable helpers returning Maybe<T>. |
| Trellis.Results/src/Maybe/Extensions/Choose.cs | Adds Choose to filter/unwrap IEnumerable<Maybe<T>>. |
| Trellis.Results/tests/Results/Extensions/MapIfTests.cs | Adds unit tests for MapIf. |
| Trellis.Results/tests/Results/Extensions/GetValueOrDefaultTests.cs | Adds unit tests for Result.GetValueOrDefault. |
| Trellis.Results/tests/Results/Extensions/EnsureTests.cs | Adds unit tests for EnsureNotNull narrowing behavior. |
| Trellis.Results/tests/Results/Extensions/CheckTests.cs | Adds unit tests for sync Check. |
| Trellis.Results/tests/Results/Extensions/CheckTests.Task.cs | Adds Task async-variant tests for CheckAsync. |
| Trellis.Results/tests/Results/Extensions/CheckTests.ValueTask.cs | Adds ValueTask async-variant tests for CheckAsync. |
| Trellis.Results/tests/Results/Extensions/BindZipTests.cs | Adds unit tests for BindZip and some async scenarios. |
| Trellis.Results/tests/Maybes/MaybeMethodTests.cs | Adds tests for new Maybe<T> instance methods. |
| Trellis.Results/tests/Maybes/MaybeEdgeCaseTests.cs | Adjusts call-site to disambiguate GetValueOrDefault overload. |
| Trellis.Results/tests/Maybes/Extensions/CollectionExtensionTests.cs | Adds tests for TryFirst/TryLast/Choose. |
| using System.Diagnostics; | ||
|
|
There was a problem hiding this comment.
using System.Diagnostics; appears to be unused in this file (no DebuggerStepThrough attribute or other System.Diagnostics types referenced explicitly). With EnforceCodeStyleInBuild=true this will trigger IDE0005 and fail the build; remove the using (or add the intended attribute/usage).
| using System.Diagnostics; |
| using System.Diagnostics; | ||
|
|
There was a problem hiding this comment.
using System.Diagnostics; appears to be unused in this file. Because code style is enforced during build, this will raise IDE0005 (unused using) and fail CI; remove the using (or add the intended DebuggerStepThrough/other diagnostics usage).
| using System.Diagnostics; |
| // T4-generated: tuple continuation overloads (2 → 9 tuples) | ||
| // e.g., Result<(T1, T2, T3)> BindZip<T1, T2, T3>(this Result<(T1, T2)>, Func<T1, T2, Result<T3>>) | ||
| // Async: BindZipAsync with Task/ValueTask Left/Right/Both variants | ||
| ``` |
There was a problem hiding this comment.
The API reference claims BindZipAsync has Task/ValueTask Left/Right/Both variants, but the code currently only provides scalar BindZipAsync for Task<Result<T>> (Left/Both) and tuple overloads where ValueTask variants are Left/Right only (no ValueTask Both, and no scalar right/ValueTask overloads). Please align the documentation with the actual overload set, or add the missing overloads so the statement is true.
| // Choose with selector | ||
| IEnumerable<string> emails = users.Choose(u => u.Email); |
There was a problem hiding this comment.
In the “Choose with selector” example, users.Choose(u => u.Email) won’t compile because Choose is defined for IEnumerable<Maybe<T>>, not IEnumerable<T>. Either change the example to call Choose on an IEnumerable<Maybe<User>> (or on users.Select(u => u.Email)), or document/add an overload that supports IEnumerable<T> + selector returning Maybe<TResult>.
| // Choose with selector | |
| IEnumerable<string> emails = users.Choose(u => u.Email); | |
| // Choose with selector (via projection) | |
| IEnumerable<string> emailsWithSelector = users | |
| .Select(u => u.Email) // Func<User, Maybe<string>> | |
| .Choose(); |
Add [GeneratedCode] attribute to BindZipTs.g.tt output classes. Simplify Directory.Build.props ExcludeByFile to **/*.g.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The IScalarValue interface requires both TryCreate(TPrimitive, string?) and TryCreate(string?, string?). The Quantity test stubs only had the primitive overload, causing a CS0535 compilation error in the generator test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| using System.Diagnostics; | ||
|
|
There was a problem hiding this comment.
using System.Diagnostics; is unused in this file. With warnings treated as errors, this unnecessary using directive will break the build (CS8019). Remove it.
| using System.Diagnostics; |
| public static partial class BindZipExtensions | ||
| { | ||
| /// <summary> | ||
| /// Binds a function to the Result value and zips both values into a tuple on success. | ||
| /// If the source or the function result is a failure, returns that failure. | ||
| /// </summary> | ||
| /// <typeparam name="T1">Type of the input result value.</typeparam> | ||
| /// <typeparam name="T2">Type of the new result value.</typeparam> | ||
| /// <param name="result">The result to bind and zip.</param> | ||
| /// <param name="func">The function to call if the result is successful.</param> | ||
| /// <returns>A tuple result combining both values on success; otherwise the failure.</returns> | ||
| public static Result<(T1, T2)> BindZip<T1, T2>( | ||
| this Result<T1> result, | ||
| Func<T1, Result<T2>> func) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(func); | ||
| using var activity = RopTrace.ActivitySource.StartActivity(); | ||
|
|
There was a problem hiding this comment.
BindZip currently only has sync, Task-left, and Task-both async variants for the 2-tuple entry point (Result<T1>). There is no Task-right overload (this Result<T1>, async func) and no ValueTask variants, even though other extensions (e.g., BindAsync) support the full Task/ValueTask matrix. Consider adding the missing overloads (Task.Right, ValueTask.Left/Right/Both) so the API is consistent and matches the documented surface.
| { | ||
| ArgumentNullException.ThrowIfNull(resultTask); | ||
| ArgumentNullException.ThrowIfNull(func); | ||
| using var activity = RopTrace.ActivitySource.StartActivity(); |
There was a problem hiding this comment.
BindZipAsync starts an activity with StartActivity() (parameterless), which will name the span after the async method (e.g., "BindZipAsync"). Elsewhere in this codebase, async variants explicitly name the activity after the sync operator (e.g., EnsureAsync uses StartActivity(nameof(EnsureExtensions.Ensure)), BindAsync uses nameof(BindExtensions.Bind)) so telemetry stays consistent across sync/async. Consider passing nameof(BindZipExtensions.BindZip) here as well.
| using var activity = RopTrace.ActivitySource.StartActivity(); | |
| using var activity = RopTrace.ActivitySource.StartActivity(nameof(BindZipExtensions.BindZip)); |
| Func<<# WriteTs(i); #>, Task<Result<T<#= outputSize #>>>> func) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(func); | ||
| using var activity = RopTrace.ActivitySource.StartActivity(); |
There was a problem hiding this comment.
In the T4 template, the async BindZipAsync overloads that create activities use StartActivity() without an explicit name. That will likely produce span names like "BindZipAsync" instead of matching the sync operator name. Other ROP async operators in this repo pin the activity name to the sync method (e.g., nameof(BindExtensions.Bind), nameof(EnsureExtensions.Ensure)), so it would be good to use StartActivity(nameof(BindZipExtensions.BindZip)) (or an equivalent constant) in the async overloads generated by this template.
| using var activity = RopTrace.ActivitySource.StartActivity(); | |
| using var activity = RopTrace.ActivitySource.StartActivity(nameof(BindZipExtensions.BindZip)); |
| public class CheckTaskTests | ||
| { |
There was a problem hiding this comment.
This file name indicates the established async-variant naming scheme used elsewhere in the repo (e.g., EnsureTests.Task.cs contains Ensure_Task_Tests). To stay consistent/readable, consider renaming the test class to match the convention for Task variants (e.g., Check_Task_Tests) and adding the usual <summary>/#region organization used in the other async extension test files.
| public class CheckValueTaskTests | ||
| { |
There was a problem hiding this comment.
This async-variant test file (*.ValueTask.cs) doesn’t follow the naming pattern used by other ValueTask variant tests in this repo (e.g., EnsureTests.ValueTask.cs contains Ensure_ValueTask_Tests). Consider renaming the class to match the convention (e.g., Check_ValueTask_Tests) and adding the standard <summary> header so it’s clear which async matrix variant the file covers.
| /// <summary> | ||
| /// Transforms the value if the result is successful and the condition is true. | ||
| /// Returns the original result unchanged if the condition is false or the result is a failure. | ||
| /// </summary> | ||
| public static Result<T> MapIf<T>(this Result<T> result, bool condition, Func<T, T> func) | ||
| { |
There was a problem hiding this comment.
These new public extension methods only have <summary> XML docs; they’re missing the usual <typeparam>, <param>, and <returns> tags that other extension methods in this project provide (e.g., Map, Ensure, ToMaybe). Adding full XML doc tags here will keep the generated docs consistent and more useful for consumers.
| /// <summary> | ||
| /// Returns the success value, or the specified default if the result is a failure. | ||
| /// This is a terminal operator that exits the Result railway. | ||
| /// </summary> | ||
| public static TValue GetValueOrDefault<TValue>(this Result<TValue> result, TValue defaultValue) => | ||
| result.IsSuccess ? result.Value : defaultValue; | ||
|
|
||
| /// <summary> | ||
| /// Returns the success value, or evaluates the factory to produce a default if the result is a failure. | ||
| /// The factory is only invoked on the failure track. | ||
| /// This is a terminal operator that exits the Result railway. | ||
| /// </summary> | ||
| public static TValue GetValueOrDefault<TValue>(this Result<TValue> result, Func<TValue> defaultFactory) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(defaultFactory); | ||
| return result.IsSuccess ? result.Value : defaultFactory(); | ||
| } |
There was a problem hiding this comment.
These new public GetValueOrDefault overloads only include <summary> docs; they’re missing the standard <typeparam>, <param>, and <returns> XML documentation tags used throughout the rest of the Result extensions. Adding those tags will improve generated API docs and keep documentation consistent.
Add Task.Right, ValueTask.Left, ValueTask.Right, and ValueTask.Both overloads for the initial BindZip entry point, matching the full async matrix provided by other extensions like Bind and Check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add missing <typeparam>, <param>, <returns>, and <exception> tags for consistency with other Result extensions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Directory.Build.props
Outdated
| @@ -37,7 +37,7 @@ | |||
|
|
|||
| <PropertyGroup Label="Coverage"> | |||
| <!-- Exclude generated regex state machines and other obj artifacts from coverage to keep metrics meaningful --> | |||
There was a problem hiding this comment.
The comment above ExcludeByFile mentions excluding "obj artifacts", but the updated pattern no longer includes an **/obj/** exclusion and now excludes all **/*.g.cs instead. Please update the comment (or the patterns) so the documentation matches what the build actually does for coverage filtering.
| <!-- Exclude generated regex state machines and other obj artifacts from coverage to keep metrics meaningful --> | |
| <!-- Exclude generated .g.cs files and regex state machine sources from coverage to keep metrics meaningful --> |
| foreach (var item in source) | ||
| { | ||
| if (item.HasValue) | ||
| yield return item.Value; | ||
| } |
There was a problem hiding this comment.
ChooseIterator checks HasValue and then accesses Value, which re-checks state internally (and routes through an exception-throwing accessor). Using TryGetValue(out var value) would avoid the double check and keep the hot-path branch-only.
| /// Extension methods for safely querying collections, returning <see cref="Maybe{T}"/> instead of throwing. | ||
| /// </summary> | ||
| [DebuggerStepThrough] | ||
| public static class MaybeTryFirstExtensions |
There was a problem hiding this comment.
The extension class is named MaybeTryFirstExtensions but it contains both TryFirst and TryLast. Renaming the type (or splitting into two types) would make discovery/intellisense and future maintenance clearer.
| public static class MaybeTryFirstExtensions | |
| public static class MaybeTryFirstLastExtensions |
The class contains both TryFirst and TryLast methods. A broader name improves discoverability and IntelliSense clarity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
HasValue + Value does two checks (and Value routes through an exception-throwing accessor). TryGetValue is a single branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CheckTests → Check_Tests, CheckTaskTests → Check_Task_Tests, CheckValueTaskTests → Check_ValueTask_Tests. Add <summary> headers documenting which async variant each file covers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pin async span names to the sync operator name, matching the convention used by BindAsync, EnsureAsync, and other ROP operators. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| /// <summary> | ||
| /// Tests for Check.Task.cs where BOTH input and function are async (Task). | ||
| /// Also covers Check.Task.Left.cs (async input, sync function) and | ||
| /// Check.Task.Right.cs (sync input, async function). | ||
| /// </summary> |
There was a problem hiding this comment.
This test file mixes Both/Left/Right variants, but other async extension tests in this repo are typically split into separate files (e.g., EnsureTests.Task.cs + EnsureTests.Task.Left.cs + EnsureTests.Task.Right.cs). Consider splitting these into separate CheckTests.Task*.cs files (and updating the XML summary) to match the existing organization and keep failures/coverage per-variant easier to locate.
| /// <summary> | ||
| /// Tests for Check.ValueTask.cs where BOTH input and function are async (ValueTask). | ||
| /// Also covers Check.ValueTask.Left.cs (async input, sync function) and | ||
| /// Check.ValueTask.Right.cs (sync input, async function). | ||
| /// </summary> |
There was a problem hiding this comment.
This file currently tests ValueTask Both/Left/Right variants together, but other async extension test suites in the repo are commonly split by variant into separate files (e.g., EnsureTests.ValueTask.cs + EnsureTests.ValueTask.Left.cs + EnsureTests.ValueTask.Right.cs). Consider splitting Check ValueTask tests similarly and adjusting the XML summary so file/variant mapping stays consistent.
| this ValueTask<Result<(<# WriteTs(i); #>)>> resultTask, | ||
| Func<<# WriteTs(i); #>, ValueTask<Result<T<#= outputSize #>>>> func) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(resultTask); |
There was a problem hiding this comment.
resultTask is a ValueTask<...> struct here, so ArgumentNullException.ThrowIfNull(resultTask) is redundant and will box the struct (unnecessary allocation). Consider removing this check and only validating func (as is done in other ValueTask-based extension overloads).
| ArgumentNullException.ThrowIfNull(resultTask); |
Result Extensions (5 new)
CheckBindZipMapIfEnsureNotNullResult<T?>→Result<T>(class + struct)GetValueOrDefaultFunc<T>, orFunc<Error, T>overloadsMaybe Instance Methods (5 new)
BindOrWhereTapGetValueOrDefault(Func<T>)Collection Helpers (3 new)
TryFirst/TryLastIEnumerable→Maybe<T>bridge (no exceptions)ChooseIEnumerable<Maybe<T>>to values