Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the repo to build/test against newer .NET TFMs (including net10.0) and modernizes build/test infrastructure and code to satisfy nullable/analyzer requirements.
Changes:
- Update projects to target
net8.0;net9.0;net10.0(dropping older TFMs) and refresh build tooling/test execution. - Apply nullable annotations and related refactors (invariant culture parsing, disposal fixes, required members, etc.).
- Add/enable analyzers and adjust warning/analysis settings across projects.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Test.Common/XunitAttributes.cs | Update Xunit attribute Skip nullability annotations. |
| test/Test.Common/TestHttpSourceResourceProvider.cs | Nullable annotations + disposal improvements in test HTTP source helpers. |
| test/Test.Common/Test.Common.csproj | Move Test.Common to net8/9/10; mark as non-test project; remove extra vuln-fix package refs. |
| test/Test.Common/CmdRunner.cs | Allow nullable env var dictionary; minor perf tweak (Count > 0). |
| test/Test.Common/CatalogReaderTestHelpers.cs | Minor perf tweak (Length > 0). |
| test/NuGetMirror.Tests/TestHttpSourceResourceProvider.cs | Dispose HttpContent backing stream. |
| test/NuGetMirror.Tests/NupkgCommandTests.cs | Update FluentAssertions API usage. |
| test/NuGetMirror.Tests/NuGetMirror.Tests.csproj | Target net8/9/10 (drop net6). |
| test/NuGetMirror.CliTool.Tests/NuGetMirror.CliTool.Tests.csproj | Target net10.0. |
| test/NuGet.CatalogReader.Tests/NuGet.CatalogReader.Tests.csproj | Target net10.0. |
| test/NuGet.CatalogReader.Tests/CatalogReaderTests.cs | Use Assert.Single for clarity. |
| src/NuGetMirror/Program.cs | Static Program + nullable HttpSource; TLS settings updated. |
| src/NuGetMirror/NupkgsCommand.cs | Nullable annotations, invariant culture parsing, token-aware delays, small refactors. |
| src/NuGetMirror/NuGetMirror.csproj | Target net8/9/10 (drop net6). |
| src/NuGetMirror/MirrorUtility.cs | Invariant culture cursor parsing; null-forgiving on reflection result. |
| src/NuGetMirror/ListCommand.cs | Nullable HttpSource + invariant culture parsing. |
| src/NuGetMirror/Constants.cs | Make constants container static. |
| src/NuGetMirror/Common/TaskUtils.cs | Make static; nullable locals; token passed to Task.Run. |
| src/NuGetMirror/Common/PatternUtils.cs | Use ArgumentNullException.ThrowIfNull. |
| src/NuGetMirror/Common/ExceptionUtils.cs | Nullable message; modern null checks; tighten nullability. |
| src/NuGetMirror/Common/CmdUtils.cs | Modern null checks; null-forgiving where framework APIs return nullable. |
| src/NuGet.CatalogValidator/ValidateCommand.cs | Nullable HttpSource; improve async usage; dispose HTTP objects; required properties. |
| src/NuGet.CatalogValidator/Program.cs | Static Program + nullable HttpSource; conditionalize ServicePointManager config. |
| src/NuGet.CatalogValidator/NuGet.CatalogValidator.csproj | Target net10.0. |
| src/NuGet.CatalogValidator/Constants.cs | Make constants container static. |
| src/NuGet.CatalogValidator/Common/ExceptionUtils.cs | Nullable message; modern null checks; improve TargetInvocationException handling. |
| src/NuGet.CatalogValidator/Common/CmdUtils.cs | Modern null checks; null-forgiving where framework APIs return nullable. |
| src/NuGet.CatalogReader/ReferenceCache.cs | Seal class; nullable-aware string caching; invariant culture date parsing. |
| src/NuGet.CatalogReader/ProcessEntriesUtility.cs | Reorder public API params; improve async consumption; filter null entries. |
| src/NuGet.CatalogReader/NuGet.CatalogReader.csproj | Target net8/9/10; remove explicit System.Text.Json package ref. |
| src/NuGet.CatalogReader/HttpReaderBase.cs | Nullable fields for deferred init; null-forgiving where initialization is guaranteed; suppress finalize on dispose. |
| src/NuGet.CatalogReader/FeedReader/SleetFeedReader.cs | Add null-forgiving for JSON property access. |
| src/NuGet.CatalogReader/FeedReader/PackageEntry.cs | Add null checks for nuspec XML; nullable signatures; add comparison operators. |
| src/NuGet.CatalogReader/FeedReader/FeedReader.cs | Null-forgiving where service index is guaranteed. |
| src/NuGet.CatalogReader/Common/TaskUtils.cs | Make static; nullable locals; token passed to Task.Run. |
| src/NuGet.CatalogReader/CatalogReaderUtility.cs | Nullable wrapper handler; tighten exception handling; nullability fixes. |
| src/NuGet.CatalogReader/CatalogReader.cs | Invariant culture parsing; nullability updates for JSON access and service index usage. |
| src/NuGet.CatalogReader/CatalogPageEntry.cs | Nullable signatures; add comparison operators. |
| src/NuGet.CatalogReader/CatalogEntry.cs | Nullable signatures for equality/compare. |
| build/config.props | Add analyzer settings; adjust PackageIconUrl formatting. |
| build/common/test.props | Disable nullable warnings for test projects. |
| build/common/packages.common.config | Remove legacy packages.config used for xunit console runner. |
| build/common/common.targets | Enable nullable by project type; bump LangVersion; add analyzers; update test execution plumbing. |
| build/common/common.shared.props | Turn on .NET analyzers/code style enforcement; remove xunit console path. |
| build/common/common.sh | Install .NET 8/9/10; restructure build/test steps; add logging helper. |
| build/common/common.ps1 | Install .NET 10; update nuget.exe download URL. |
| build/common/build.shared.proj | Escape/unescape semicolon-separated test project paths; run all tests via dotnet vstest. |
| ReleaseNotes.md | Add 4.0.0 entry for net10 support/removal of net6.0. |
Comments suppressed due to low confidence (4)
src/NuGet.CatalogReader/FeedReader/PackageEntry.cs:301
CompareToreturns-1whenotheris null. ForIComparable<T>, the conventional behavior is that any non-null instance compares greater than null (return1). With the newly added relational operators, the current implementation can produce inconsistent results (e.g., bothx < nullandnull < xcan evaluate true). Consider changing the null branch to return1so ordering is consistent and transitive.
public int CompareTo(PackageEntry? other)
{
if (other == null)
{
return -1;
}
src/NuGet.CatalogReader/CatalogPageEntry.cs:64
CompareToreturns-1whenotheris null. ForIComparable<T>, the conventional behavior is that any non-null instance compares greater than null (return1). With the newly added relational operators below, the current null ordering can lead to inconsistent comparisons. Consider returning1whenotheris null so ordering is consistent.
public int CompareTo(CatalogPageEntry? other)
{
if (other == null)
{
return -1;
}
src/NuGet.CatalogReader/CatalogEntry.cs:113
CompareToreturns-1whenotheris null. ForIComparable<T>, the conventional behavior is that a non-null instance compares greater than null (return1). Returning-1can break sort ordering assumptions in callers.
public int CompareTo(CatalogEntry? other)
{
if (other == null)
{
return -1;
}
src/NuGet.CatalogValidator/Program.cs:82
- The
#if NET8_0guard means this entire block will be excluded when targeting net10.0 (the project now targets net10.0), soDefaultConnectionLimitandSecurityProtocolwill no longer be set. If the intent is to apply this configuration to net8+ (including net10), consider using an *_OR_GREATER symbol or removing the conditional.
#if NET8_0
// Set connection limit
if (!RuntimeEnvironmentHelper.IsMono)
{
ServicePointManager.DefaultConnectionLimit = 64;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static bool operator ==(CatalogPageEntry? left, CatalogPageEntry? right) => left?.Equals(right) ?? right is null; | ||
| public static bool operator !=(CatalogPageEntry? left, CatalogPageEntry? right) => !(left == right); | ||
| public static bool operator <(CatalogPageEntry? left, CatalogPageEntry? right) => left is null ? right is not null : left.CompareTo(right) < 0; | ||
| public static bool operator <=(CatalogPageEntry? left, CatalogPageEntry? right) => left is null || left.CompareTo(right) <= 0; | ||
| public static bool operator >(CatalogPageEntry? left, CatalogPageEntry? right) => left is not null && left.CompareTo(right) > 0; | ||
| public static bool operator >=(CatalogPageEntry? left, CatalogPageEntry? right) => left is null ? right is null : left.CompareTo(right) >= 0; |
There was a problem hiding this comment.
The newly added relational operators depend on CompareTo, but with the current CompareTo(null) behavior they can yield contradictory results for null comparisons. After adjusting CompareTo's null handling, verify these operators still implement a strict weak ordering (null typically sorts before non-null).
| public static Task<IReadOnlyList<FileInfo>> DownloadNuspecsAsync(string outputDirectory, DownloadMode mode, int maxConcurrentDownloads, IEnumerable<CatalogEntry> entries, CancellationToken token) | ||
| { |
There was a problem hiding this comment.
These public API methods changed parameter order to move CancellationToken to the end. That’s a source/binary breaking change for any external callers. If the intent is to improve the API shape while keeping compatibility, consider adding overloads that preserve the previous parameter order (and optionally marking them obsolete) instead of changing the existing signatures.
build/config.props
Outdated
| <PackageIconUrl> | ||
| https://emgartenstatic.blob.core.windows.net/nupkgs/icons/nuget.catalogreader.png</PackageIconUrl> |
There was a problem hiding this comment.
PackageIconUrl is now defined with embedded newlines/indentation. MSBuild property values preserve this whitespace, which can result in an invalid/whitespace-prefixed URL in the generated NuGet metadata. Consider keeping the URL on a single line (or otherwise ensuring the value is trimmed) to avoid publishing a malformed URL.
| <PackageIconUrl> | |
| https://emgartenstatic.blob.core.windows.net/nupkgs/icons/nuget.catalogreader.png</PackageIconUrl> | |
| <PackageIconUrl>https://emgartenstatic.blob.core.windows.net/nupkgs/icons/nuget.catalogreader.png</PackageIconUrl> |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5af81edf4
ℹ️ 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".
|
|
||
| public static bool operator ==(CatalogPageEntry? left, CatalogPageEntry? right) => left?.Equals(right) ?? right is null; | ||
| public static bool operator !=(CatalogPageEntry? left, CatalogPageEntry? right) => !(left == right); | ||
| public static bool operator <(CatalogPageEntry? left, CatalogPageEntry? right) => left is null ? right is not null : left.CompareTo(right) < 0; |
There was a problem hiding this comment.
Fix contradictory null ordering in page entry operators
The new comparison operators can produce impossible results when one operand is null: CatalogPageEntry.CompareTo(null) returns -1, so nonNull < null evaluates true, while the null fast-path in operator < also makes null < nonNull true. This breaks antisymmetry and can corrupt any caller logic that relies on <, <=, > or >= for ordering/filtering when nulls are present.
Useful? React with 👍 / 👎.
|
|
||
| public static bool operator ==(PackageEntry? left, PackageEntry? right) => left?.Equals(right) ?? right is null; | ||
| public static bool operator !=(PackageEntry? left, PackageEntry? right) => !(left == right); | ||
| public static bool operator <(PackageEntry? left, PackageEntry? right) => left is null ? right is not null : left.CompareTo(right) < 0; |
There was a problem hiding this comment.
Fix contradictory null ordering in package entry operators
The added relational operators have the same null-ordering contradiction as above: PackageEntry.CompareTo(null) returns -1, so nonNull < null is true, and operator < also returns true for null < nonNull. This makes the ordering relation inconsistent and can lead to incorrect behavior in consumer code that compares package entries with nullable operands.
Useful? React with 👍 / 👎.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
No description provided.