Develop#20
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refactored TryActionResult to use regular constructor instead of primary constructor parameters to: - Eliminate redundant property initializations that shadowed parameters - Ensure compatibility with older .NET frameworks (netstandard2.0, net462) - Maintain consistency with TryFuncResult pattern - Add proper XML documentation for the main constructor Co-authored-by: Chris-Wolfgang <210299580+Chris-Wolfgang@users.noreply.github.com>
[WIP] Address feedback from review on initial implementation
Co-authored-by: Chris-Wolfgang <210299580+Chris-Wolfgang@users.noreply.github.com>
Remove redundant property initializations in TryActionResult
# Conflicts: # tests/TryPattern.Tests/TryPattern.Tests.csproj
…sualstudio-3.1.5'
…sualstudio-3.1.5' into develop # Conflicts: # tests/TryPattern.Tests/TryPattern.Tests.csproj
…est.Sdk-18.0.1' into develop # Conflicts: # tests/TryPattern.Tests/TryPattern.Tests.csproj
Co-authored-by: Chris-Wolfgang <210299580+Chris-Wolfgang@users.noreply.github.com>
Added TryActionResultTests.cs and TryFuncResultTests.cs to cover constructors and property assignments for both result classes. Tests include various scenarios (success, failure, exceptions) and multiple data types, using xUnit for comprehensive coverage and improved reliability.
Added TryPattern.Tests.csproj with xUnit and coverage tools for .NET 8.0. The test project references the main TryPattern project. Also removed the explicit <Version> tag from Wolfgang.TryPattern.csproj.
Added --output-file devskim-results.txt to the DevSkim analyze command in the workflow, ensuring scan results are saved to a file for easier access and review. No other workflow changes were made.
- Add continue-on-error to DevSkim scan to prevent workflow failure on findings - Exclude **/TestResults/** from scan via ignore-globs - Always show DevSkim results, with fallback message if no output - Set artifact upload to warn if results file is missing
Refactored Wolfgang.TryPattern.Tests.csproj to use conditional ItemGroups for framework-specific test dependencies, improving maintainability and compatibility. Updated TryFuncResult.cs constructor formatting for consistency (tabs instead of spaces) with no logic changes. Removed net9.0 from main project target frameworks; test project still targets net9.0. No functional code changes.
Added build-and-test-windows job to pr.yaml for building and testing on Windows with .NET 3.1–9.0, including code coverage and artifact upload. Also introduced build-and-test-macos job for .NET 5+ on macOS. Both jobs skip execution in the template repository.
- Refactored GitHub Actions to use matrix builds for Ubuntu, Windows, and macOS - Added .NET 10.0.x support across workflows - Improved error handling for label creation; added "dependencies" and "dotnet" labels - Enhanced DocFX workflow with build output verification - Separated build/test logic for Linux/macOS and Windows; coverage reporting now Windows-only - Improved artifact uploads with OS-specific naming - Security scanning now runs on all platforms with better reporting and failure detection
Major overhaul of pr.yaml GitHub Actions workflow: - Split into 3 gated stages: Linux (with 90% coverage gate), then Windows/macOS, then .NET Framework 4.x on Windows - Only proceed to next stage if previous passes - Coverage enforcement now on Linux, with clear per-module threshold - Artifacts for coverage and build outputs uploaded per stage - Security scan (DevSkim) runs in parallel with improved output - Workflow ignores docs/markdown changes for PR triggers - Improved modularity, logs, and error handling for maintainability
Added a step in the GitHub Actions workflow to install libssl1.1 on Ubuntu 22.04+ for the test-linux-core job. This ensures .NET Core 3.1 compatibility, as libssl1.1 is not included by default in newer Ubuntu versions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Removed .NET Core 3.1 from the test matrix in the CI pipeline, so only net5.0 through net10.0 are tested. Added an informational step to echo a message on macOS runners explaining the skip of .NET Core 3.1 due to lack of ARM64 support, and clarifying that it is still tested on Linux and Windows.
[WIP] Address feedback from review on pull request #14
[WIP] Address feedback from PR #14 review comments
Co-authored-by: Chris-Wolfgang <210299580+Chris-Wolfgang@users.noreply.github.com>
[WIP] Address feedback on 'Develop' PR for improvements
…-time [WIP] WIP address feedback from review on develop PR
Updated Wolfgang.TryPattern.csproj to set the project version to 0.1.0-rc1, marking this build as a release candidate. No other changes were made.
There was a problem hiding this comment.
Pull request overview
This PR represents a major refactoring and restructuring of the TryPattern library. The namespace has been renamed from TryPattern to Wolfgang.TryPattern, and the API has been fundamentally redesigned to return result objects instead of raw values, providing better exception handling visibility.
Key changes:
- API redesign:
Try.Action()andTry.Function()now returnTryActionResultandTryFuncResult<T>objects respectively, exposingSucceeded,Failed, andExceptionproperties - Multi-framework targeting: Test project now targets 9 different frameworks from .NET Framework 4.6.2 to .NET 9.0 (and incorrectly, .NET 10.0)
- Project restructuring: Solution reorganized with new examples folder, improved test coverage, and updated CI/CD workflows
Reviewed changes
Copilot reviewed 25 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Wolfgang.TryPattern.Tests/Wolfgang.TryPattern.Tests.csproj | New multi-targeted test project configuration supporting .NET Framework 4.6.2 through .NET 10.0 with framework-specific package versions |
| tests/Wolfgang.TryPattern.Tests/TryFunctionTests.cs | Comprehensive tests for synchronous function execution with new result object assertions |
| tests/Wolfgang.TryPattern.Tests/TryFunctionAsyncTests.cs | Tests for async function execution covering various scenarios including exceptions and nullable types |
| tests/Wolfgang.TryPattern.Tests/TryFuncResultTests.cs | Unit tests validating the new TryFuncResult record behavior across different constructors and types |
| tests/Wolfgang.TryPattern.Tests/TryActionTests.cs | Updated tests for action execution to validate new TryActionResult return type |
| tests/Wolfgang.TryPattern.Tests/TryActionResultTests.cs | Tests for the new TryActionResult record type |
| tests/Wolfgang.TryPattern.Tests/TryActionAsyncTests.cs | Updated async action tests to verify result objects instead of exception swallowing |
| tests/TryPattern.Tests/TryPattern.Tests.csproj | Old test project with package version downgrades (appears to be a compatibility fallback) |
| tests/TryPattern.Tests/TryFunctionTests.cs | Old test file removed (migrated to Wolfgang.TryPattern.Tests) |
| tests/TryPattern.Tests/TryFunctionAsyncTests.cs | Old async test file removed (migrated to Wolfgang.TryPattern.Tests) |
| src/Wolfgang.TryPattern/Wolfgang.TryPattern.csproj | New library project targeting .NET Framework 4.6.2, .NET Standard 2.0, .NET 8.0, and .NET 10.0 with package metadata |
| src/Wolfgang.TryPattern/TryFuncResult.cs | New record type representing function execution results with conditional compilation for nullable support |
| src/Wolfgang.TryPattern/TryActionResult.cs | New record type representing action execution results |
| src/Wolfgang.TryPattern/Try.cs | Refactored Try class returning result objects instead of raw values/void |
| src/TryPattern/TryPattern.csproj | Old project file removed |
| src/TryPattern/Try.cs | Old implementation removed (replaced by Wolfgang.TryPattern version) |
| examples/Wolfgang.TryPattern.Example1/Wolfgang.TryPattern.Example1.csproj | New example console application project |
| examples/Wolfgang.TryPattern.Example1/Program.cs | Placeholder example program |
| benchmarks/Wolfgang.TryPattern.Benchmarks/Wolfgang.TryPattern.Benchmarks.csproj | Updated benchmark project reference to new library name |
| benchmarks/Wolfgang.TryPattern.Benchmarks/TryBenchmarks.cs | Updated namespace to Wolfgang.TryPattern.Benchmarks |
| benchmarks/Wolfgang.TryPattern.Benchmarks/Program.cs | Updated using statement for new namespace |
| TryPattern.sln | Solution file updated with new project structure including examples, docs, and GitHub workflow references |
| .github/workflows/pr.yaml | Updated macOS testing to skip .NET Core 3.1 and .NET 5.0 due to ARM64 compatibility, with improved documentation |
| .github/workflows/docfx.yaml | Updated DocFX workflow to use .NET 10.0 SDK and added build verification step |
| .github/workflows/create-labels.yaml | Enhanced error handling for label creation with additional labels for dependencies and dotnet |
Comments suppressed due to low confidence (1)
tests/Wolfgang.TryPattern.Tests/TryActionAsyncTests.cs:109
- There's a spacing inconsistency in this assertion. There should be no space between
3and,callCount. The corrected line should beAssert.Equal(3, callCount);to match the formatting style used throughout the test file.
| <PropertyGroup Condition=" | ||
| '$(TargetFramework)' == 'net5.0' OR | ||
| '$(TargetFramework)' == 'net6.0' OR | ||
| '$(TargetFramework)' == 'net7.0' OR | ||
| '$(TargetFramework)' == 'net8.0' OR | ||
| '$(TargetFramework)' == 'net9.0' OR | ||
| '$(TargetFramework)' == 'net10.0' | ||
| "> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
The conditional compilation directive references NET5_0_OR_GREATER, but this target framework list includes net462, netstandard2.0, and net8.0. The net8.0 framework will satisfy NET5_0_OR_GREATER, but net10.0 (which doesn't exist yet) is also included. There's a mismatch between the conditional at line 26-35 which checks for net5.0-net10.0 and the actual target frameworks defined. Consider ensuring the conditional compilation directives align with the actual target frameworks being used.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/Wolfgang.TryPattern.Tests/TryActionAsyncTests.cs:109
- There is a spacing error before the comma. It should be "callCount)" instead of "callCount )".
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Chris-Wolfgang I've opened a new pull request, #21, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/Wolfgang.TryPattern.Tests/TryActionAsyncTests.cs:109
- There is a spacing error before the comma. The space should come after the comma, not before it.
replace with <Description>A lightweight .NET library implementing the Try pattern for safe, exception-free operations across multiple target frameworks.</Description> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
tests/Wolfgang.TryPattern.Tests/TryActionAsyncTests.cs:109
- Missing space before the comma in the assertion. Should be "Assert.Equal(3, callCount)" for consistent spacing.
| #if NET5_0_OR_GREATER | ||
| public static async Task<TryFuncResult<T?>> FunctionAsync<T>(Func<Task<T>> function) | ||
| #else | ||
| public static async Task<TryFuncResult<T>> FunctionAsync<T>(Func<Task<T>> function) | ||
| #endif |
There was a problem hiding this comment.
The conditional compilation directives for return types create inconsistent API surface across different target frameworks. For .NET 5.0 and later, the method returns TryFuncResult<T?>, while earlier frameworks return TryFuncResult. This inconsistency could lead to confusion and portability issues for consumers targeting multiple frameworks. Consider using a consistent return type across all frameworks or clearly documenting this behavioral difference.
| /// <summary> | ||
| /// Gets the result value, if available. | ||
| /// </summary> | ||
| public T? Value { get; } |
There was a problem hiding this comment.
The conditional compilation for the Value property at line 97 makes it nullable (T?) for all frameworks, but the constructor parameters use conditional compilation to make T nullable only for NET5_0_OR_GREATER. This creates an inconsistency where the Value property is always nullable, but the constructor parameter type changes based on framework. This could lead to unexpected null handling behavior across different target frameworks.
| /// <summary> | ||
| /// Executes an asynchronous action and swallows any exceptions that occur. | ||
| /// </summary> | ||
| /// <param name="action">The asynchronous action to execute.</param> | ||
| /// <returns>A task representing the asynchronous operation.</returns> |
There was a problem hiding this comment.
The XML documentation states "Executes an asynchronous action and swallows any exceptions that occur", but the method no longer swallows exceptions silently - it returns a TryActionResult object containing exception information. The documentation should be updated to reflect that it captures exceptions and returns them in the result object rather than swallowing them.
| /// <summary> | ||
| /// Executes a function and returns its result, or the default value of T if an exception occurs. | ||
| /// </summary> | ||
| /// <typeparam name="T">The return type of the function.</typeparam> | ||
| /// <param name="function">The function to execute.</param> | ||
| /// <returns>The result of the function, or default(T) if an exception occurs.</returns> |
There was a problem hiding this comment.
The XML documentation states the method "returns the result of the function, or default(T) if an exception occurs", but the method now returns a TryFuncResult object, not the raw value or default(T). The documentation needs to be updated to reflect that it returns a result object containing the value, exception, and success status.
| #if NET5_0_OR_GREATER | ||
| public static TryFuncResult<T?> Function<T>(Func<T> function) | ||
| #else | ||
| public static TryFuncResult<T> Function<T>(Func<T> function) | ||
| #endif |
There was a problem hiding this comment.
The conditional compilation directives for return types create inconsistent API surface across different target frameworks. For .NET 5.0 and later, the method returns TryFuncResult<T?>, while earlier frameworks return TryFuncResult. This inconsistency could lead to confusion and portability issues for consumers targeting multiple frameworks. Consider using a consistent return type across all frameworks or clearly documenting this behavioral difference.
| /// <summary> | ||
| /// Executes an asynchronous function and returns its result, or the default value of T if an exception occurs. | ||
| /// </summary> | ||
| /// <typeparam name="T">The return type of the function.</typeparam> | ||
| /// <param name="function">The asynchronous function to execute.</param> | ||
| /// <returns>A task representing the asynchronous operation that returns the result of the function, or default(T) if an exception occurs.</returns> |
There was a problem hiding this comment.
The XML documentation states the method "returns a task representing the asynchronous operation that returns the result of the function, or default(T) if an exception occurs", but the method now returns Task<TryFuncResult>, not the raw value or default(T). The documentation needs to be updated to reflect that it returns a result object containing the value, exception, and success status.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/Wolfgang.TryPattern.Tests/TryActionAsyncTests.cs:109
- Missing space after comma. The code has "Assert.Equal(3 ,callCount)" but should be "Assert.Equal(3, callCount)" for consistent spacing.
Description
Fixes/Complete # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist
Screenshots (if applicable)
Additional context