feat(utilities): add TryResult helpers - #112
Conversation
|
@coderabbitai autofix |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughA new 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Add author-side TryResult helpers that initialize out parameters while returning the surrounding TryX result value. Cover bool failure and success paths for one, two, and three out parameters, plus non-bool sentinel failure returns. Add focused tests for defaulting, value assignment, non-nullable value-type out parameters, and composition inside a try-parse method.
6c7cd6a to
40b1b84
Compare
|
@coderabbitai autofix |
ANcpLua
left a comment
There was a problem hiding this comment.
Self-review findings: no blocking issues in this PR diff.
Checked scope:
- PR now contains only
src/ANcpLua.Roslyn.Utilities/TryResult.csandtests/ANcpLua.Roslyn.Utilities.Testing.Tests/TryResultTests.cs. - Removed the stale already-merged workflow/npm commits from the compare by rebasing onto current
origin/main. - Tightened XML docs to avoid claiming guaranteed zero-overhead or identical IL.
- Added coverage for non-nullable value-type
outparameters, nullable outs, multi-out failure paths, non-bool sentinel returns, success assignment, and real try-parse composition.
Validation:
git diff --check origin/main...HEAD- focused TryResult test project passed: 27 total, 0 failed
- Release solution build passed with 0 warnings and 0 errors
- runtime and source-only packages packed; source package contains
TryResult.cs
Known existing issue, not introduced here: full dotnet test --solution ANcpLua.Roslyn.Utilities.slnx fails in ANcpLua.Roslyn.Utilities.ExtensibleEnumMirror.Tests with generated namespace TestNs.TestNs; / CS8955, and the same failure reproduces on detached origin/main.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c7cd6ac5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// <param name="result">When this method returns, contains <c>default(T)</c>.</param> | ||
| /// <returns><c>false</c>.</returns> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool Fail<T>(out T? result) |
There was a problem hiding this comment.
Use non-nullable out parameter in Fail helpers
The Fail API currently takes out T?, which makes common try-pattern signatures noisy or warning-prone in nullable-enabled projects: calling TryResult.Fail(out result) from methods that expose out string result (or generic out T result) produces nullability-mismatch diagnostics and forces callers to either relax their public signature to nullable or suppress warnings. This undercuts the intended one-liner helper for standard [MaybeNullWhen(false)] out T try methods, especially when warnings are treated as errors.
Useful? React with 👍 / 👎.
Summary
TryResult, an author-side helper for compactbool TryX(out T)success and failure branches.outparameters for failure and success paths, plus non-bool sentinel return values.outparameters, and composition inside a readable try-parse method.origin/main; the stale workflow and npm churn commits are already upstream and no longer part of this diff.Validation
git diff --check origin/main...HEADdotnet test --project tests/ANcpLua.Roslyn.Utilities.Testing.Tests/ANcpLua.Roslyn.Utilities.Testing.Tests.csprojpassed: 27 total, 0 failed.dotnet restore ANcpLua.Roslyn.Utilities.slnxdotnet build ANcpLua.Roslyn.Utilities.slnx -c Release --no-restore -p:Version=0.0.0-localdotnet pack src/ANcpLua.Roslyn.Utilities/ANcpLua.Roslyn.Utilities.csproj -c Release -o artifacts/pr-pack -p:Version=0.0.0-local --no-builddotnet pack src/ANcpLua.Roslyn.Utilities.Sources/ANcpLua.Roslyn.Utilities.Sources.csproj -c Release -o artifacts/pr-pack -p:Version=0.0.0-local --no-buildANcpLua.Roslyn.Utilities.Sources.0.0.0-local.nupkgcontainscontentFiles/cs/any/ANcpLua.Roslyn.Utilities/TryResult.cs.Known Existing Failure
dotnet test --solution ANcpLua.Roslyn.Utilities.slnxstill fails inANcpLua.Roslyn.Utilities.ExtensibleEnumMirror.Testsbecause generated output isnamespace TestNs.TestNs;and hits CS8955. This reproduces on detachedorigin/mainwithdotnet test --project tests/ANcpLua.Roslyn.Utilities.ExtensibleEnumMirror.Tests/ANcpLua.Roslyn.Utilities.ExtensibleEnumMirror.Tests.csproj, so it is not introduced by this PR.Summary by CodeRabbit
Release Notes