Brighter icon indicating copy to clipboard operation
Brighter copied to clipboard

feat: Add support to .NET v10

Open lillo42 opened this issue 2 months ago โ€ข 8 comments

Add support for .NET V10

For now, I've removed code to support MySQL EF Core from the sample because it doesn't have support for .NET V10. Once this PR https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/pull/2019 is merged and the package is generated, I'll undo my changes on the sample

For MongoDB EF Core I'll keep on .NET 8 & 9, waiting for https://jira.mongodb.org/projects/EF/issues/EF-272

lillo42 avatar Oct 07 '25 13:10 lillo42

Pull Request Review: .NET 10 Support

Thank you for this PR! Adding .NET 10 support is a significant undertaking. Here's my comprehensive review:


โœ… Positive Aspects

1. Comprehensive Scope

  • All 80 changed files have been updated systematically
  • Samples, tests, and source projects all migrated consistently
  • CI/CD workflow properly updated for .NET 10 preview

2. Good Architectural Decisions

  • Using BrighterTargetFramework property to centralize target framework management is excellent
  • Conditional package references based on TargetFramework maintains backward compatibility
  • Multi-targeting approach (netstandard2.0;net8.0;net9.0;net10.0) allows gradual adoption

3. Package Management

  • Properly using centralized package management via Directory.Packages.props
  • Preview packages (rc.2) are appropriately versioned for .NET 10

๐Ÿ› Critical Issues

1. Typo in System.Text.Json Package References (Lines 138 & 159 in Directory.Packages.props)

<!-- CURRENT (INCORRECT) -->
<PackageVersion Include="System.Text.Json" ersion="9.0.9" />
<PackageVersion Include="System.Text.Json" ersion="10.0.0-rc.2.25502.107" />

<!-- SHOULD BE -->
<PackageVersion Include="System.Text.Json" Version="9.0.9" />
<PackageVersion Include="System.Text.Json" Version="10.0.0-rc.2.25502.107" />

Missing 'V' in 'Version' attribute - This will cause build failures.

Location: Directory.Packages.props:138 and Directory.Packages.props:159


โš ๏ธ Concerns & Questions

2. CI/CD Changes May Break Existing .NET 8/9 Testing

The workflow now only installs .NET 10:

dotnet-version: '10.x.x'
dotnet-quality: 'preview'

Problem: If the codebase still multi-targets net8.0 and net9.0 (which it does according to src/Directory.Build.props:32), you need those SDKs installed to build and test those target frameworks.

Recommendation: Either:

  • Keep multi-SDK installation:
    dotnet-version: |
      8.0.x
      9.0.x
      10.x.x
    dotnet-quality: 'preview'
    
  • OR document that you're dropping .NET 8/9 from CI (though source still targets them)

3. Pomelo.EntityFrameworkCore.MySql Not Updated for .NET 10

Line 189 in Directory.Packages.props:

<PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" />

While other EF Core packages are using 10.0.0-rc.2, Pomelo is still at 9.0.0. This might cause compatibility issues if EF Core 10 has breaking changes.

Check: Does Pomelo 9.0.0 work with EF Core 10.0.0-rc.2? You may need to watch for a Pomelo preview release.

4. Microsoft.Extensions.TimeProvider.Testing Version Discrepancy

Line 158:

<PackageVersion Include="Microsoft.Extensions.TimeProvider.Testing" Version="9.9.0" />

This is using version 9.9.0 for .NET 10, while other Microsoft.Extensions.* packages are at 10.0.0-rc.2. Is there a reason, or is a 10.0.0 preview not available yet?

5. Assembly Version Updated to 10.0.0

src/Directory.Build.props:26-29 shows assembly versions updated to 10.0.0. This is a breaking semantic version change.

Questions:

  • Is this intentional for .NET 10 support, or should this be a minor/patch version?
  • Are there breaking API changes that warrant a major version bump?
  • Should this be coordinated with the .NET 10 release timeline?

6. [WIP] Tag - What's Remaining?

The PR is marked as [WIP]. What tasks remain before this is ready for merge?

Suggested checklist:

  • [ ] Fix System.Text.Json typos
  • [ ] Verify all tests pass on .NET 10
  • [ ] Resolve CI/CD multi-targeting concerns
  • [ ] Verify third-party package compatibility (Pomelo, etc.)
  • [ ] Update documentation/release notes
  • [ ] Performance testing on .NET 10

๐Ÿ“‹ Best Practices Alignment

โœ… Follows Repository Guidelines:

  • Central package management via Directory.Packages.props โœ“
  • Consistent target framework management โœ“
  • All Microsoft.Extensions.* packages aligned within each TFM โœ“

โš ๏ธ Potential Concerns:

  • Mixing preview (.NET 10 RC) with stable packages (some dependencies) - acceptable for preview work but note for production
  • No test coverage changes visible - are there new tests for .NET 10-specific features?

๐Ÿ”’ Security & Performance

  • No security concerns identified - this is infrastructure/build configuration
  • Performance considerations: .NET 10 typically brings performance improvements; no regressions expected
  • Preview/RC quality means thorough testing needed before production use

๐Ÿ“ Recommendations

High Priority:

  1. Fix the System.Text.Json typos immediately - blocking issue
  2. Clarify CI/CD multi-targeting strategy - may cause build failures
  3. Verify Pomelo MySQL provider compatibility

Medium Priority:

  1. Document what's remaining for WIP โ†’ ready
  2. Consider adding CI job that tests all target frameworks (not just net10.0)
  3. Update CHANGELOG/release notes for .NET 10 support

Nice to Have:

  1. Add smoke tests specifically targeting .NET 10 features if applicable
  2. Document minimum SDK version required for contributors

๐ŸŽฏ Summary

This is a well-structured PR for .NET 10 support with good architectural decisions. The main blockers are:

  1. Critical typo in package references (easy fix)
  2. CI/CD strategy needs clarification

Once these are addressed, the PR should be in good shape! The systematic approach to updating all projects is commendable.

Overall Assessment: โš ๏ธ Needs fixes before merge, but solid foundation


Let me know if you have questions about any of these points! Happy to discuss the multi-targeting strategy or help validate package compatibility.

claude[bot] avatar Oct 20 '25 08:10 claude[bot]

Pull Request Review: .NET 10 Support

Overview

This PR adds support for .NET 10 to the Brighter project. The changes are systematic and well-structured, covering build configuration, dependency management, CI/CD pipeline updates, and project files across the entire codebase.

Positive Aspects

1. Systematic Approach

  • Consistent pattern: All 78 .csproj files updated with $(BrighterTargetFramework) variable
  • Centralized configuration: Smart use of Directory.Build.props to define the target framework globally
  • Multi-targeting support: The src/Directory.Build.props properly defines multi-targeting with net8.0;net9.0;net10.0

2. Dependency Management

  • Framework-specific packages: Properly conditional package versions for .NET 8, 9, and 10
  • Consistent version updates: All Microsoft.Extensions.* and System.* packages aligned to version 10.0.0 for .NET 10
  • EF Core versioning: Correct separate versioning for EF Core packages across frameworks (8.0.22, 9.0.10, 10.0.0)

3. CI/CD Updates

  • Simplified configuration: Moved from multi-version matrix to single .NET 10 preview
  • Consistent quality setting: All jobs now use dotnet-quality: 'preview'
  • Updated tooling: dotnet-ef updated to 10.0.0

Issues and Concerns

1. CRITICAL: Missing Pomelo.EntityFrameworkCore.MySql Package

The Pomelo.EntityFrameworkCore.MySql package has been removed from Directory.Packages.props but no explanation or migration path is provided:

  • Lines removed: Original file had Pomelo package for .NET 8 and 9
  • Impact: This will break MySQL/MariaDB Entity Framework Core support
  • Action required:
    • Either restore the package with .NET 10 compatible version
    • Or document the migration strategy if MySQL support is being dropped
    • Or add to known limitations if .NET 10 version isn't available yet

Location: Directory.Packages.props:165-172 (net8.0), 173-180 (net9.0)

2. Dependency Management Violation

According to docs/agent_instructions/dependency_management.md:33:

"Avoid mixing preview and stable package versions."

This PR mixes:

  • Preview packages: All .NET 10 Microsoft packages at version 10.0.0 (preview)
  • Stable packages: Third-party packages like MongoDB.Driver, RabbitMQ.Client, etc.

Recommendation: This is likely acceptable for a WIP preview support PR, but should be documented in the PR description. Consider adding a note about which packages are preview vs stable.

3. Pre-release Dependency

Npgsql is using a release candidate version:

<PackageVersion Include="Npgsql" Version="10.0.0-rc.1" />

Considerations:

  • RC versions can have breaking changes before final release
  • May need to monitor for Npgsql 10.0.0 stable release
  • Consider adding a TODO or tracking issue

4. CI/CD Simplification May Be Too Aggressive

The CI changed from testing multiple .NET versions to only .NET 10:

# Before
dotnet-version: |
  8.0.x
  9.0.x

# After  
dotnet-version: '10.x.x'
dotnet-quality: 'preview'

Concerns:

  • No longer testing .NET 8 or .NET 9 compatibility
  • Since the code still targets net8.0, net9.0, and net10.0, the CI should verify all targets
  • Breaking changes in .NET 10 won't be caught for older frameworks

Recommendation: Consider either:

  • Testing all three frameworks (8, 9, 10) in CI
  • Or explicitly document that CI only validates latest framework
  • Matrix strategy could test net10.0 primarily with periodic net8.0/9.0 checks

5. TUnit Package Removed

Directory.Packages.props previously included:

<PackageVersion Include="TUnit" Version="0.67.10" />

This has been removed without explanation:

  • Is TUnit no longer needed?
  • Was it experimental and never used?
  • Should this be documented in the PR?

6. Whitespace Inconsistency

Minor formatting issue in CI file (.github/workflows/ci.yml:155):

--health-retries 5             # Extra trailing spaces

Changed to:

--health-retries 5  # Better, but still has trailing spaces

Not critical, but shows opportunity for cleanup.

Security Considerations

  • โœ… No security concerns identified
  • โœ… No credentials or secrets exposed
  • โœ… Dependency versions are from official sources

Performance Considerations

  • โœ… No performance impact from configuration changes
  • โ„น๏ธ .NET 10 may have performance improvements, but that's framework-level
  • โ„น๏ธ Preview builds may have different performance characteristics

Test Coverage

Major Concern: The PR title says [WIP] but there's no indication of:

  • What specific .NET 10 features are being tested
  • Whether existing tests pass on .NET 10
  • Any new .NET 10-specific issues discovered

Recommendation: Before removing WIP status:

  1. Document test results across all platforms
  2. Add notes about any .NET 10-specific behavior changes
  3. Verify all test suites pass with .NET 10 SDK

Code Quality

  • โœ… Follows the project's centralized package management pattern
  • โœ… Consistent use of MSBuild properties
  • โœ… No code logic changes (only configuration)
  • โš ๏ธ Missing documentation of breaking changes or limitations

Recommendations

Must Fix Before Merge

  1. Restore or document Pomelo.EntityFrameworkCore.MySql removal
  2. Document preview package usage in PR description
  3. Address CI multi-targeting strategy

Should Consider

  1. Add a CHANGELOG or migration guide entry
  2. Document any known .NET 10 preview limitations
  3. Add tracking issue for Npgsql RC dependency
  4. Verify all samples build and run on .NET 10
  5. Consider a compatibility matrix in documentation

Nice to Have

  1. Clean up trailing whitespace in CI file
  2. Document TUnit package removal
  3. Add .NET 10 testing instructions to CONTRIBUTING.md

Conclusion

This is a solid foundation for .NET 10 support with systematic changes across the codebase. The main concerns are:

  1. The missing MySQL EF Core provider (critical)
  2. CI/CD strategy for multi-targeting
  3. Documentation of preview/RC dependencies

The structural approach is excellent and follows the project's patterns well. Once the critical issues are addressed and documentation is added, this will be ready for merge.

Overall Assessment: Good start, but needs attention to dependency completeness and CI strategy before removing WIP status.

claude[bot] avatar Nov 11 '25 20:11 claude[bot]

Pull Request Review: Add .NET 10 Support

Thank you for working on adding .NET 10 support to Brighter! This is an important update. I've reviewed the changes and have some feedback organized by category.


๐Ÿ”ด Critical Issues

1. .NET 10 Package Availability

Location: Directory.Packages.props

The PR references .NET 10 packages, but as of January 2025, .NET 10 has not been released yet. The expected timeline is:

  • .NET 10 Preview: Expected early 2025
  • .NET 10 GA: Expected November 2025

Issue: Many packages in Directory.Packages.props reference version 10.0.0 which don't exist yet:

  • Microsoft.EntityFrameworkCore 10.0.0
  • Microsoft.Extensions.* 10.0.0
  • Npgsql 10.0.0
  • System.Text.Json 10.0.0
  • TickerQ 10.0.0
  • Microsoft.EntityFrameworkCore.MySql 10.0.0

Impact: This PR will fail to build because these packages don't exist on NuGet yet.

Recommendation:

  • Either wait until .NET 10 preview packages are available, or
  • Use conditional package references that only apply when building against .NET 10 once it's available
  • Monitor NuGet for package availability

โš ๏ธ Major Concerns

2. CI/CD Workflow Testing

Location: .github/workflows/ci.yml

The workflow adds 10.0.x to all test jobs, which is good for future-proofing. However:

Issue: The CI will fail immediately because:

  • .NET 10 SDK isn't available yet for GitHub Actions
  • The workflow doesn't have conditional logic to skip .NET 10 tests if the SDK isn't available

Recommendation:

- name: Setup .NET
  uses: actions/setup-dotnet@v4
  with:
    dotnet-version: |
      8.0.x
      9.0.x
  continue-on-error: true  # Allow to continue if 10.0.x isn't available

Or use a matrix strategy with conditional execution for .NET 10.

3. Package Version Management Strategy

Location: Directory.Packages.props

The current approach uses framework-specific <ItemGroup> conditions, which is good. However:

Issues:

  1. The condition logic has a problem:
<ItemGroup Condition="'$(TargetFramework)' == 'net10.0' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net472'">

This groups .NET 10 with .NET Framework and netstandard2.0, which seems incorrect. .NET 10 should have its own section separate from legacy frameworks.

  1. Some packages may not support netstandard2.0 at version 10.0.0

Recommendation: Separate .NET 10 into its own conditional block:

<ItemGroup Condition="'$(TargetFramework)' == 'net10.0'">
  <!-- .NET 10 specific packages -->
</ItemGroup>

4. Missing Package Version Updates

Location: Directory.Packages.props

While .NET 8 and 9 packages were updated to latest patch versions, several framework-agnostic packages should also be updated:

  • MySqlConnector: 2.4.0 โ†’ 2.5.0 (good! โœ“)
  • OpenTelemetry.*: Updated (good! โœ“)
  • Microsoft.NET.Test.Sdk: 18.0.0 โ†’ 18.0.1 (good! โœ“)

But consider also updating:

  • Microsoft.Data.SqlClient (currently 6.1.2)
  • Other non-framework-specific packages

๐Ÿ“ Minor Issues & Suggestions

5. Assembly Version Bumping

Location: src/Directory.Build.props

<AssemblyVersion>10.0.0.0</AssemblyVersion>
<FileVersion>10.0.0.0</FileVersion>
<InformationalVersion>10.0.0</InformationalVersion>
<Version>10.0.0</Version>

Issue: The assembly version was bumped to 10.0.0, but this may be premature if you're only adding .NET 10 support rather than making breaking changes.

Recommendation: Consider whether this warrants a major version bump according to your versioning policy. Adding framework support is typically a minor version change unless there are breaking API changes.

6. Documentation

Issue: No updates to documentation explaining .NET 10 support status, compatibility matrix, or migration guide.

Recommendation: Add or update:

  • README.md with .NET 10 support status
  • Release notes mentioning .NET 10 support
  • Migration guide if there are any breaking changes

7. Removed Package References

Location: Various .csproj files

Some projects had direct package references removed (e.g., in Paramore.Brighter.Mediator.csproj, Paramore.Brighter.csproj). This appears to be consolidation into Directory.Packages.props.

Observation: This is good for centralized package management, but ensure all projects still compile correctly with transitive dependencies.

8. Whitespace Changes

Location: .github/workflows/ci.yml:160

Minor formatting change (trailing space removed). Good cleanup! โœ“


โœ… Positive Aspects

  1. Consistent approach: All sample projects updated consistently with variable reference
  2. Centralized versioning: Good use of Directory.Packages.props for version management
  3. Framework-specific packages: Proper separation of package versions by target framework
  4. CI coverage: .NET 10 added to all test jobs for comprehensive testing
  5. Tool updates: dotnet-ef updated to 10.0.0
  6. Multi-targeting: Proper support for net8.0, net9.0, and net10.0 simultaneously

๐Ÿ”ง Code Quality & Best Practices

According to the repository's coding standards (from CLAUDE.md):

โœ… Follows conventions:

  • Proper use of MSBuild properties
  • Consistent file structure
  • Clear separation of concerns in package management

โš ๏ธ Areas for improvement:

  • Testing guidelines suggest comprehensive test coverage - ensure .NET 10-specific tests are added once SDK is available
  • Documentation standards require updates to reflect new framework support

๐Ÿ›ก๏ธ Security Considerations

No security concerns identified. The changes are primarily configuration updates for framework support.


๐ŸŽฏ Performance Considerations

.NET 10 is expected to bring performance improvements. Once released:

  • Monitor for JIT improvements
  • Take advantage of new runtime features
  • Review any deprecated APIs

๐Ÿ“‹ Action Items

Before merging:

  1. โš ๏ธ BLOCKER: Wait for .NET 10 preview packages to be available on NuGet
  2. โš ๏ธ BLOCKER: Verify the PR builds successfully with .NET 10 SDK
  3. Update conditional logic in Directory.Packages.props to separate .NET 10 from legacy frameworks
  4. Add documentation about .NET 10 support status
  5. Consider whether assembly version should be 10.0.0 or follow existing versioning scheme
  6. Add release notes entry
  7. Run full test suite against .NET 10 once SDK is available

Recommended approach: Given that .NET 10 hasn't been released yet, consider:

  1. Creating a feature branch that tracks .NET 10 preview releases
  2. Updating as preview packages become available
  3. Merging after .NET 10 GA and thorough testing

๐Ÿ“Š Summary

Overall Assessment: This PR demonstrates good preparation for .NET 10 support with a well-structured approach to multi-targeting. However, it's premature as .NET 10 hasn't been released yet. The PR cannot be merged in its current state due to missing packages.

Recommendation:

  • Mark as Draft/WIP (which it already is โœ“)
  • Monitor .NET 10 preview releases
  • Update incrementally as packages become available
  • Plan for thorough testing once SDK is available

Risk Level: Low (once packages are available) Code Quality: Good Architecture: Sound approach to multi-targeting


Great work on being proactive with .NET 10 support! Once the preview packages are available, this will be ready to proceed with testing. ๐Ÿ‘

claude[bot] avatar Nov 24 '25 13:11 claude[bot]

Pull Request Review: .NET 10 Support

This is a substantial infrastructure update. Below is my detailed review.

Critical Issues

  1. Incorrect Package Version Conditionals (Directory.Packages.props:182-214) - The conditional groups net10.0 with legacy frameworks (netstandard2.0, net462, net472), causing legacy frameworks to incorrectly receive .NET 10 package versions which will cause build failures. Split into separate conditions.

  2. Non-Existent Package Versions - Several packages reference version 10.0.0 which do not exist yet: Microsoft.EntityFrameworkCore.MySql and Npgsql version 10.0.0 do not exist.

  3. Hardcoded NotSupported Blocks - Startup.cs:156 and Program.cs:177 throw NotSupportedException for MySQL which breaks functionality.

Major Concerns

  1. Missing Test Coverage - No verification that tests pass on .NET 10
  2. Version Number Inconsistency - Assembly version bumped to 10.0.0 - is this intentional for just framework support?
  3. Incomplete EntityFrameworkCore Migration - No migration path for MySQL documented

Action Items (Must Fix Before Merge)

  1. Fix conditional logic in Directory.Packages.props (critical)
  2. Verify all package versions exist
  3. Fix or remove MySql NotSupported blocks
  4. Run complete test suite on .NET 10
  5. Decide on assembly version strategy

Summary

Excellent approach using centralized package management. However, critical package versioning issues must be fixed before merge. The conditional logic error will cause build failures on legacy frameworks.

Recommendation: Request Changes

claude[bot] avatar Nov 26 '25 12:11 claude[bot]

@lillo42 Is this ready for review?

iancooper avatar Dec 05 '25 12:12 iancooper

@iancooper it's ready, I'll resolve the conflicts

lillo42 avatar Dec 08 '25 11:12 lillo42

Pull Request Review: .NET 10 Support

Thank you for this comprehensive update to add .NET 10 support! This is a substantial PR that touches 88 files with well-organized changes. Here's my detailed review:

โœ… Strengths

  1. Comprehensive Coverage: The PR systematically updates all samples and test projects across the entire codebase.

  2. Centralized Configuration: Using BrighterTargetFramework property in Directory.Build.props is an excellent approach for managing framework versions centrally.

  3. Framework-Specific Package Management: The conditional package versioning in Directory.Packages.props is well-structured, properly handling different package versions for .NET 8, 9, and 10.

  4. CI/CD Updates: All GitHub Actions workflows properly include .NET 10 in the matrix builds.

  5. Transparent Communication: Clear documentation in the PR description about the temporary MySQL EF Core limitation.

๐Ÿ” Issues & Concerns

1. Package Version Inconsistencies (Critical)

In Directory.Packages.props, there's a problematic condition on line 163 that groups .NET 10 with .NET Framework 4.6.2, 4.7.2, and .NET Standard 2.0, meaning these older frameworks will attempt to use .NET 10 packages (version 10.0.0), which will fail.

Impact: This will break builds for projects targeting .NET Framework or .NET Standard 2.0.

Fix: The .NET 10 condition should be separate from legacy framework conditions.

2. Missing MongoDB.EntityFrameworkCore for .NET 10

In Directory.Packages.props, the .NET 10 section is missing MongoDB.EntityFrameworkCore (present in .NET 8 and 9 sections). If this package is needed, it should be included.

3. Version Downgrade

Swashbuckle.AspNetCore was downgraded from 10.0.1 to 9.0.6. If this is intentional due to compatibility issues with .NET 10, it should be documented. Otherwise, consider keeping the latest version.

4. Package Version Inconsistency - TickerQ

The package TickerQ appears with version 2.5.3 in the general section, but this seems inconsistent with the framework-specific sections. Verify which version should be used for .NET 10.

5. Sample Code - NotSupportedException Handling

In samples/WebAPI/WebAPI_EFCore/GreetingsWeb/Startup.cs:156 and SalutationAnalytics/Program.cs:177, throwing NotSupportedException in production code paths could cause runtime failures. Consider adding guard conditions earlier in the configuration and using more specific exception messages.

๐Ÿงช Testing Considerations

  1. Test Coverage: No new tests were added, but this is acceptable for a framework upgrade. Existing tests should validate the migration.

  2. CI Matrix: Excellent that all test jobs now include .NET 10 in the matrix.

  3. Verification Needed:

    • Ensure all test suites pass with .NET 10
    • Verify package restore works correctly for all target frameworks
    • Test that .NET Framework and .NET Standard 2.0 projects still build

๐Ÿ“‹ Minor Observations

  1. Npgsql Version: For .NET 9, Npgsql was upgraded from 8.0.7 to 9.0.4. This is fine but worth noting.

  2. OpenTelemetry.Exporter.Jaeger: Updated to 1.6.0-rc.1 (pre-release). Verify this doesn't introduce stability issues.

  3. Project Files: The consistent use of BrighterTargetFramework across all samples is excellent and follows the DRY principle.

๐ŸŽฏ Recommendations

  1. CRITICAL: Fix the conditional package version grouping in Directory.Packages.props line 163.

  2. HIGH: Add missing MongoDB.EntityFrameworkCore package for .NET 10 if needed.

  3. MEDIUM: Document or justify the Swashbuckle downgrade.

  4. MEDIUM: Review exception handling approach for MySQL EF Core limitation.

  5. LOW: Consider separating the whitespace cleanup into a separate commit per Tidy First guidelines.

โœ… Security & Performance

  • No security concerns identified
  • No performance regressions expected
  • Package updates appear appropriate

๐Ÿ“ Summary

This is a well-structured PR with comprehensive coverage of the .NET 10 migration. The main blocker is the package version condition issue that could break legacy framework builds. Once that is resolved, this should be good to merge (pending successful CI test runs).

Recommendation: Request changes to fix the critical package versioning issue, then approve after verification.


Great work on this migration! Let me know if you would like me to clarify any of these points.

claude[bot] avatar Dec 08 '25 15:12 claude[bot]

Pull Request Review: Add support to .NET v10

Summary

This PR successfully adds .NET 10.0 support to the Brighter project. The changes are well-structured and comprehensive, updating CI workflows, package dependencies, and sample projects.

โœ… Strengths

  1. Comprehensive Dependency Management: The Directory.Packages.props refactoring is excellent - moving from global version specifications to framework-specific conditional groups makes dependency management much cleaner and maintainable.

  2. CI/CD Coverage: All test workflows in .github/workflows/ci.yml have been updated to include .NET 10.0, ensuring consistent test coverage across frameworks.

  3. Centralized Configuration: Introduction of BrighterTargetFramework property in Directory.Build.props provides a single point of control for all sample projects, reducing duplication.

  4. Pragmatic Approach: Temporarily disabling MySQL EF Core support with clear NotSupportedException messages and comments is the right approach while waiting for upstream dependencies.

๐Ÿ” Code Quality Observations

Good Practices Observed:

  • Clear exception messages explaining why operations are not supported
  • Consistent use of the centralized $(BrighterTargetFramework) property across all sample projects
  • Framework-specific package versioning properly organized

Areas for Consideration:

  1. Package Versioning Strategy (samples/WebAPI/WebAPI_EFCore/Startup.cs:156 and SalutationAnalytics/Program.cs:177):

    • The temporary MySQL EF Core workaround uses NotSupportedException with a message about waiting for Pomelo support
    • Consider: This is appropriate for samples, but ensure this is documented in release notes so users know about this limitation
    • The commented-out code should ideally be tracked in an issue and removed once support is restored
  2. Conditional Compilation (Directory.Packages.props:125):

    • The condition '$(TargetFramework)' == 'net10.0' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net472' groups .NET 10 with legacy frameworks
    • Rationale: This appears intentional to use the latest package versions for .NET 10 and legacy frameworks, but it's somewhat counterintuitive
    • Consider: Adding a comment explaining why net10.0 is grouped with legacy frameworks (presumably because they all want the latest versions)
  3. Version Consistency:

    • OpenTelemetry.Exporter.Jaeger upgraded to 1.6.0-rc.1 (a pre-release version)
    • Consider: Document why a pre-release version is necessary (likely for .NET 10 compatibility)
  4. Missing Package in Migration:

    • Microsoft.EntityFrameworkCore.MySql is added to the net10.0 condition with version 10.0.0
    • This appears premature since the code explicitly throws NotSupportedException for MySQL
    • Consider: Remove this package reference until the underlying Pomelo package is ready

๐Ÿ›ก๏ธ Security & Performance

No Security Concerns Identified: The changes are primarily infrastructure and dependency updates with no security implications.

No Performance Concerns: The package updates should maintain or improve performance as they include latest optimizations from .NET 10.

๐Ÿ“‹ Test Coverage

Test Coverage Adequate:

  • All 15 test workflow jobs updated to include .NET 10.0
  • Tests will run against all supported frameworks (8.0, 9.0, 10.0)

Consider: Once MySQL support is restored, ensure integration tests cover all database types with .NET 10.

๐Ÿ› Potential Issues

  1. Minor: The Microsoft.EntityFrameworkCore.MySql package version 10.0.0 in Directory.Packages.props may not exist yet, which could cause restore failures if someone tries to use MySQL

    • Impact: Low - code will throw NotSupportedException anyway
    • Suggestion: Either remove the package reference or add a comment that it's a placeholder
  2. Whitespace Change (.github/workflows/ci.yml:178): There's a trailing whitespace cleanup (purely cosmetic)

๐Ÿ“ Suggestions

  1. Documentation: Add a comment in Directory.Packages.props explaining the framework grouping logic

    <!-- .NET 10.0 is grouped with legacy frameworks to use latest package versions -->
    <ItemGroup Condition="'$(TargetFramework)' == 'net10.0' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net472'">
    
  2. Tracking: Create issues to track:

    • Re-enabling MySQL EF Core support when Pomelo package is released
    • Upgrading OpenTelemetry.Exporter.Jaeger from RC to stable release
  3. PR Description: The PR body mentions the Pomelo PR link - consider adding an issue reference in the code comments for easy tracking

โœจ Overall Assessment

Status: โœ… Approve with minor suggestions

This is a solid PR that successfully adds .NET 10.0 support in a well-organized manner. The dependency management refactoring improves code organization significantly. The temporary MySQL limitation is handled appropriately with clear error messages.

The suggestions above are minor improvements and don't block merging. Great work on maintaining consistency across all sample projects and test suites!


Code adheres to Brighter conventions: โœ… Tests updated appropriately: โœ…
Breaking changes: None Documentation needed: Release notes should mention MySQL EF Core limitation

claude[bot] avatar Dec 08 '25 16:12 claude[bot]