Enable nullable on NuGet.Protocol legacy feed types (phase 20)#7498
Open
nkolev92 wants to merge 1 commit into
Open
Enable nullable on NuGet.Protocol legacy feed types (phase 20)#7498nkolev92 wants to merge 1 commit into
nkolev92 wants to merge 1 commit into
Conversation
Remove #nullable disable from all phase 20 files and add honest nullable annotations: - V2FeedPackageInfo: most string properties → string? (XML source), Authors/Owners stay non-null (Array.Empty fallback) - V2FeedParser: GetPackage returns V2FeedPackageInfo?, nullable params for id and SourceCacheContext - V2FeedQueryBuilder: private helpers return string?, searchTerm nullable - V2FeedPage: NextUri → string? - PackageSearchMetadataV2Feed: nullable URIs, Description, Summary, Tags; Title stays non-null (PackageId fallback) - ODataServiceDocumentResourceV2: add ArgumentNullException guard for baseAddress - ODataServiceDocumentResourceV2Provider: TryCreate returns INuGetResource? - Update PublicAPI.Shipped.txt for both net472 and net8.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jebriede
approved these changes
Jun 23, 2026
jebriede
left a comment
jebriede
left a comment
Contributor
There was a problem hiding this comment.
Approving with one suggestion
| public ODataServiceDocumentResourceV2(string baseAddress, DateTime requestTime) | ||
| { | ||
| _baseAddress = baseAddress.Trim('/'); | ||
| _baseAddress = (baseAddress ?? throw new ArgumentNullException(nameof(baseAddress))).Trim('/'); |
Contributor
There was a problem hiding this comment.
This guard actually changes behavior: before, a null baseAddress would throw an NullReferenceException when .Trim('/') ran, and now it throws ArgumentNullException instead. That's a good change, but it's the only behavioral change in what's otherwise an annotation-only PR, and the checklist shows no tests were added. Should we add a small test, or at least mention it in the PR remarks? That way the difference from the old behavior is clearly intentional and covered.
zivkan
approved these changes
Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Fixes: NuGet/Home#13836
Description
Phase 20 of the NuGet.Protocol nullable migration: enable nullable reference types on legacy V2 feed types.
Changes
string?(XML source), Authors/Owners stay non-null (Array.Emptyfallback),MinClientVersion→NuGetVersion?GetPackagereturnsV2FeedPackageInfo?, nullable params foridandSourceCacheContextstring?,searchTermnullableNextUri→string?ArgumentNullExceptionguard forbaseAddressTryCreatereturnsINuGetResource?PublicAPI.Shipped.txtfor both net472 and net8.0 (~70 entries each)null→null!inV2FeedQueryBuilderTestsforArgumentNullExceptiontestNote that IPackageSearchMetadata is not yet annotated. The idea is we annotate the implementation first and then we basically OR all of those values to annotate IPackageSearchMetadata correctly.
PR Checklist