Skip to content

Conversation

@SimonCropp
Copy link
Contributor

No description provided.

Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request Changes

Technical Review: ⚠️ Major Concern - This is a massive change (100 files, 4,632 deletions). While the modernization to C# 11+ ArgumentNullException.ThrowIfNull() is technically correct, this scope requires careful review.

Process Issues:

  • Missing Issue Link: This PR must be linked to an issue describing the modernization effort
  • Insufficient Description: PR description should explain the scope and benefits
  • CI Status: Build failures detected - must fix before review

Recommendation:

  1. Fix the build failures first
  2. Create an issue describing the .NET modernization effort
  3. Add comprehensive description explaining the changes
  4. Ensure all tests pass

SimonCropp and others added 2 commits October 7, 2025 17:40
Update assertion parameter name from "namespace" to "@namespace" to match actual parameter
@devlead devlead force-pushed the use-ArgumentNullException.ThrowIfNull branch from a579272 to 178b5a9 Compare October 7, 2025 15:47
@devlead devlead linked an issue Oct 7, 2025 that may be closed by this pull request
@devlead devlead merged commit 38e804f into cake-build:develop Oct 7, 2025
15 of 16 checks passed
This was referenced Nov 11, 2025
This was referenced Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace manual null checks with ArgumentNullException.ThrowIfNull()

2 participants