v3.7.0 #65
v3.7.0 #65
Conversation
Updated .editorconfig to disable specific naming rule warnings. Refactored exception classes to include remarks and simplify constructors. Changed return type of GetEnumMemberValue to nullable string. Refactored StringExtensions to use var for local variables. Expanded CountryCodeHelper to map three-digit country codes. Removed new() constraint from GetAsync<T> in INhlApiHttpClient. Made HttpClient property nullable in INhlApiHttpClient. Refactored NhlApiHttpClient constructor to use property initializers. Added null checks and exception handling in GetAsync<T>. Replaced ServicePointManager settings with property initializers. Updated HttpClient usages to handle potential null values. Replaced object with Lock for thread synchronization. Added #pragma directives to suppress nullable warnings. Updated TryGetAsync<T> in ICachingService to return nullable type. Simplified initialization of ConcurrentDictionary in CachingService. Updated NhlApiAsyncHelper to use simplified TaskFactory initialization. Updated model classes to use required keyword for properties. Targeted .NET 9.0 and incremented version to 3.7.0 in project file. Removed redundant using directives and added necessary ones. Simplified initialization of static fields in various classes. Added CultureInfo.InvariantCulture to string operations. Refactored tests to improve consistency and readability. Added InvalidTeamAbbreviationException for error handling.
Updated .NET version from 8.0.x to 9.0.x in the setup step of GitHub Actions workflows across multiple YAML files (`develop-build.yml`, `master-build-daily.yml`, `master-build.yml`, and `publish-nuget.yml`).
Updated `develop-build.yml` and `master-build.yml`:
- Added `-m` option to `dotnet test` command.
- Upgraded Codecov action from `v3` to `v5`.
- Added `fail_ci_if_error: false` and `token: ${{ secrets.CODECOV_TOKEN }}` to Codecov action.
Updated `publish-nuget.yml`:
- Added `-m` option to `dotnet test` command.
📝 WalkthroughWalkthroughThis pull request introduces several significant updates to the NHL API library, primarily focusing on enhancing type safety, improving error handling, and adding new functionality. The changes span multiple projects within the solution, including Changes
Sequence DiagramsequenceDiagram
participant Client
participant NhlApi
participant NhlLeagueApi
participant PlayoffSeriesScheduleService
Client->>NhlApi: GetPlayoffSeriesBySeasonYearAsync(seasonYear)
NhlApi->>NhlLeagueApi: GetPlayoffSeriesBySeasonYearAsync(seasonYear)
NhlLeagueApi->>PlayoffSeriesScheduleService: Retrieve Playoff Series
PlayoffSeriesScheduleService-->>NhlLeagueApi: Return PlayoffSeriesSchedule
NhlLeagueApi-->>NhlApi: Return PlayoffSeriesSchedule
NhlApi-->>Client: Return PlayoffSeriesSchedule
Possibly Related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #65 +/- ##
==========================================
+ Coverage 89.25% 89.81% +0.55%
==========================================
Files 75 78 +3
Lines 3435 3652 +217
Branches 438 476 +38
==========================================
+ Hits 3066 3280 +214
+ Misses 301 294 -7
- Partials 68 78 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add new test methods and modify existing test method Modified `GetGoalieSeasonGameLogsBySeasonAndGameTypeAsync_Test_PlayerId_Fails_Season_Year_Empty` to remove `seasonYear` parameter. Added new test methods to verify correct behavior and exception handling for `GetGoalieInformationAsync`, `DisposeAsync`, `Dispose`, and `GetPlayerDraftRankingByYearAsync`.
Enhanced error messages in NhlApiHttpClient for null HTTP responses. Updated PlayerTests to expect HttpRequestException for invalid inputs. Added default case in StatisticsTests to handle unexpected playerEnum values. Introduced new tests in StatisticsTests for various scenarios in GetSkaterStatisticsLeadersAsync, GetRealtimePlayerStatisticsBySeasonAndFilterExpressionAsync, and GetTimeOnIcePlayerStatisticsBySeasonAndFilterExpressionAsync methods.
- Added a `break` statement in `TestMethodWithRetryAttribute` to exit the loop when a condition is met. - Introduced new test methods in `LeagueTests` to test `GetPlayoffSeriesBySeasonYearAsync` with valid and invalid season years. - Updated `README.md` to document the new `GetPlayoffSeriesBySeasonYearAsync` method in `NhlApi` and `NhlLeagueApi`. - Added `GetPlayoffSeriesBySeasonYearAsync` to `NhlApi` to fetch the NHL playoff schedule for a given season year. - Updated `INhlLeagueApi` interface to include `GetPlayoffSeriesBySeasonYearAsync`. - Implemented `GetPlayoffSeriesBySeasonYearAsync` in `NhlLeagueApi` with season year validation and NHL web API retrieval. - Created `PlayoffSeriesSchedule` and related classes (`Seed`, `BottomSeed`, `TopSeed`, `Round`, `Series`) to represent playoff series schedule data.
Added GetPlayerSeasonGameLogsBySeasonAndGameTypeAsync_Test_InvalidSeasonYear to PlayerTests. - Decorated with TestMethodWithRetry to retry up to 5 times. - Uses DataRow to test invalid inputs: " " and "202520262027". - Asserts ArgumentException is thrown for invalid inputs. - Ensures method handles invalid season year formats correctly.
The `Broadcast.cs` and `TeamStatisticsDetails.cs` files have been completely removed. The `Broadcast` class included properties such as `Id`, `Name`, `Type`, `Site`, and `Language`, all annotated with `JsonProperty` attributes for JSON serialization. The `TeamStatisticsDetails` class contained numerous properties related to NHL team statistics, such as `GamesPlayed`, `Wins`, `Losses`, `Points`, `GoalsPerGame`, and others, also annotated with `JsonProperty` attributes.
Refactored NhlGameService to use concise object initialization with `new()` syntax for `_nhlScoresHtmlReportsApiHttpClient`. Changed `CalculateMultiplier` to a private static method. Added a `default` case in `PlayerTests` switch statement to throw `ArgumentException` for unmatched `query` values.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (1)
921-923:⚠️ Potential issueBug in takeaway statistic tracking.
This appears to be a pre-existing bug, but in the
takeawaycase, the code is incorrectly incrementing theGiveawaystatistic instead of theTakeawaystatistic.- gameStatisticTotals[PlayerGameCenterStatistic.Takeaway] = play.Details.PlayerId == playerId - ? gameStatisticTotals[PlayerGameCenterStatistic.Giveaway] += 1 - : gameStatisticTotals[PlayerGameCenterStatistic.Giveaway]; + gameStatisticTotals[PlayerGameCenterStatistic.Takeaway] = play.Details.PlayerId == playerId + ? gameStatisticTotals[PlayerGameCenterStatistic.Takeaway] += 1 + : gameStatisticTotals[PlayerGameCenterStatistic.Takeaway];
🧹 Nitpick comments (19)
Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (1)
541-541: Unnecessary null-forgiving operator.The null-forgiving operator (
!) aftergameTypeseems unnecessary sincegameTypeis not marked as nullable in the method signature on line 465. Consider removing this operator if null reference types are properly configured in the project.- .Where(g => g.GameType == (int)gameType!) + .Where(g => g.GameType == (int)gameType)Nhl.Api.Tests/ProjectStructureTests.cs (1)
85-85: Inconsistent use ofthisqualifier.The code adds
this.qualifier to_whiteListedFilesaccess, which is inconsistent with line 53 wherethis.was removed fromRootDirectoryFolder. Consider maintaining consistent style throughout the class.- foreach (var file in files.Where(f => !this._whiteListedFiles.Contains(f.Name))) + foreach (var file in files.Where(f => !_whiteListedFiles.Contains(f.Name)))Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs (2)
8-13: Consider passing message to base Exception constructor instead of overriding MessageThe current implementation overrides the
Messageproperty instead of passing the message to the baseExceptionconstructor. This differs from the pattern used in other exception classes likeInvalidPlayerPositionException.-public class InvalidTeamAbbreviationException(string message) : Exception -{ - /// <summary> - /// The exception message - /// </summary> - public override string Message => message; -} +public class InvalidTeamAbbreviationException(string message) : Exception(message) +{ +}This change would maintain consistency with other exception classes in the codebase and leverage the base implementation correctly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-8: Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs#L8
Added line #L8 was not covered by tests
[warning] 13-13: Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs#L13
Added line #L13 was not covered by tests
1-14: Add test coverage for the new exception classStatic analysis indicates this new class lacks test coverage. Consider adding unit tests to verify the exception behaves as expected.
Would you like me to provide a sample test that covers this exception class?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-8: Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs#L8
Added line #L8 was not covered by tests
[warning] 13-13: Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs#L13
Added line #L13 was not covered by testsNhl.Api.Common/Exceptions/InvalidPlayerPositionException.cs (1)
7-9: Consider keeping<summary>instead of using<remarks>The documentation has been moved from
<summary>to<remarks>, but this is an unusual use of the<remarks>tag which is typically for additional information beyond the summary.-/// <remarks> -/// An exception for when a request is made to the Nhl.Api and the player/goalie position is invalid -/// </remarks> +/// <summary> +/// An exception for when a request is made to the Nhl.Api and the player/goalie position is invalid +/// </summary>Nhl.Api.Common/Exceptions/NhlApiRequestException.cs (1)
7-10: Good modernization with C# primary constructor pattern.Converting to a primary constructor simplifies the code while maintaining the same functionality. However, there's inconsistency in documentation - you're now using both
<summary>(line 4-6) and<remarks>(line 7-9) tags with identical content.Consider removing the redundant documentation:
/// <summary> /// An exception for a failed Nhl.Api HTTP request /// </summary> -/// <remarks> -/// An exception for a failed Nhl.Api HTTP request -/// </remarks> public class NhlApiRequestException(string message) : Exception(message)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: Nhl.Api.Common/Exceptions/NhlApiRequestException.cs#L10
Added line #L10 was not covered by testsNhl.Api/README.md (4)
163-165: New TOC Entry in NhlApi Section:
A new Table of Contents entry has been added for
[GetPlayoffSeriesBySeasonYearAsync(seasonYear,cancellationToken)](…)in the NhlApi section. Please verify that the method signature (both the parameter names and order) is consistent with the implementation. Also, note that the summary in the detailed section currently contains a typo (“playofff” instead of “playoff”).
227-229: New TOC Entry in NhlLeagueApi Section:
A matching TOC entry for
[GetPlayoffSeriesBySeasonYearAsync(seasonYear,cancellationToken)](…)has been added in the NhlLeagueApi section. Please check for consistency with the NhlApi entry and ensure that both method references and descriptions are in sync with the actual code implementation.
1056-1072: Heading Hierarchy and Summary Typos in NhlApi Detailed Documentation:
The detailed documentation forGetPlayoffSeriesBySeasonYearAsync(in the NhlApi section) appears to be reporting a heading‐level issue (static analysis indicates “Expected: h4; Actual: h5”). Please review the heading levels so they increment sequentially (for example, if the parent section is h3, then the method’s heading should be h4 rather than h5). In addition, the summary text currently reads “Returns the current NHL playofff schedule for the current season” – please remove the extra “f” so it reads “playoff schedule.”A possible diff suggestion to fix these issues might be:
-##### Summary +#### Summaryand
-Returns the current NHL playofff schedule for the current season +Returns the current NHL playoff schedule for the current seasonPlease also verify that any link fragments (e.g. those appearing at line 1070) resolve correctly.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1058-1058: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5(MD001, heading-increment)
1070-1070: Link fragments should be valid
null(MD051, link-fragments)
2222-2236: Heading Hierarchy and Link Fragment in NhlLeagueApi Detailed Documentation:
Similarly, in the NhlLeagueApi section the documentation forGetPlayoffSeriesBySeasonYearAsync(around line 2222) is triggering a heading increment warning (expected h4 but appears as h5) and a link fragment warning (MD051 indicates an invalid or “null” link fragment). Please adjust the heading so that it follows the established hierarchy (for example, change from five hashes to four hashes if needed) and verify that the anchor links are valid.
A diff example might be:-##### Summary +#### SummaryAlso, check the URL fragment provided (for example, after the method name in the TOC and the anchor in the detailed documentation) so that it correctly matches the target heading in the document.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
2222-2222: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5(MD001, heading-increment)
2234-2234: Link fragments should be valid
null(MD051, link-fragments)
Nhl.Api.Tests/CachingServiceTests.cs (1)
11-11: Consider using interface type for better testing practicesChanging from
ICachingServiceto concreteCachingServicetype reduces abstraction. While this works for these tests, programming to interfaces generally provides better flexibility for future changes and mocking.-CachingService service = new(); +ICachingService service = new CachingService();Also applies to: 25-25, 45-45
Nhl.Api.Common/Http/NhlScoresHtmlReportsApiHttpClient.cs (1)
12-14: Added pragma directives to handle non-nullable reference type warnings.The warnings suppression addresses the compiler warnings related to non-nullable field initialization. Consider using the 'required' modifier as suggested by the warning message for a more permanent solution.
-#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. - private static HttpClient _httpClient; -#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. + private static HttpClient? _httpClient;Nhl.Api.Common/Http/NhlSuggestionApiHttpClient.cs (1)
12-14: Added pragma directives to handle non-nullable reference type warnings.The warnings suppression addresses the compiler warnings related to non-nullable field initialization. Consider using the 'required' modifier as suggested by the warning message for a more permanent solution.
-#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. - private static HttpClient _httpClient; -#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. + private static HttpClient? _httpClient;Nhl.Api.Common/Http/NhlStaticAssetsApiHttpClient.cs (1)
12-14: Added pragma directives to suppress CS8618 warningThe warning suppression handles the non-nullable field initialization warning appropriately. Consider making the field nullable instead of using pragma directives for a more type-safe approach.
-#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. -private static HttpClient _httpClient; -#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. +private static HttpClient? _httpClient;Nhl.Api.Common/Http/NhlEApiHttpClient.cs (1)
12-14: Added pragma directives to suppress CS8618 warningThe warning suppression handles the non-nullable field initialization warning appropriately. Consider making the field nullable instead of using pragma directives for a more type-safe approach.
-#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. -private static HttpClient _httpClient; -#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring as nullable. +private static HttpClient? _httpClient;Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (1)
419-419: Be cautious with the null-forgiving operator.While it works here due to the comprehensive switch statement above, the null-forgiving operator (
!) should be used carefully as it can mask potential null reference exceptions.Consider adding a default value assignment instead of using the null-forgiving operator:
-return await Task.FromResult(teamColors!); +return await Task.FromResult(teamColors ?? new TeamColors { PrimaryColor = "#000000", SecondaryColor = "#FFFFFF" });Nhl.Api.Common/Http/NhlApiHttpClient.cs (1)
109-111: Consider type-safe deserialization approachThe warning suppression for CS8603 (possible null reference return) suggests that
JsonConvert.DeserializeObject<T>could return null. Consider using a more type-safe approach instead of suppressing the warning.-#pragma warning disable CS8603 // Possible null reference return. - return JsonConvert.DeserializeObject<T>(contentResponse); -#pragma warning restore CS8603 // Possible null reference return. + var result = JsonConvert.DeserializeObject<T>(contentResponse); + return result ?? throw new JsonSerializationException($"Failed to deserialize the response to type {typeof(T).Name}");Nhl.Api.Domain/Models/Player/PlayerShiftInformation.cs (1)
14-14: Improved model validation with required propertiesAdding the
requiredmodifier strengthens the contracts of these classes. Consider whether usingobjecttype forEventDescriptionandEventDetails(lines 61 and 67) is the optimal approach. While they're now required, using a more specific type might provide better type safety thanobject.Also applies to: 48-48, 55-55, 61-61, 67-67, 81-81, 95-95, 102-102, 130-130, 137-137, 151-151
Nhl.Api.Domain/Models/Game/GameCenterLanding.cs (1)
1025-1078: Consider addingrequiredmodifier to nullable properties in the ThreeStar class.While most reference-type properties in the file have been updated with the
requiredmodifier, the properties in the ThreeStar class (like TeamAbbrev, Headshot, Name, etc.) are still nullable without the required modifier. For consistency, consider either:
- Adding the
requiredmodifier and removing the nullable annotation if these properties should never be null- Keep them nullable if these values can legitimately be missing
- public string? TeamAbbrev { get; set; } + public required string TeamAbbrev { get; set; } - public string? Headshot { get; set; } + public required string Headshot { get; set; } - public string? Name { get; set; } + public required string Name { get; set; } - public string? FirstName { get; set; } + public required string FirstName { get; set; } - public string? LastName { get; set; } + public required string LastName { get; set; } - public string? Position { get; set; } + public required string Position { get; set; }Note: Only apply this change if these properties are indeed required and should never be null in your domain model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
.github/workflows/codeql-analysis.ymlis excluded by!**/*.yml.github/workflows/develop-build.ymlis excluded by!**/*.yml.github/workflows/master-build-daily.ymlis excluded by!**/*.yml.github/workflows/master-build.ymlis excluded by!**/*.yml.github/workflows/publish-nuget.ymlis excluded by!**/*.yml
📒 Files selected for processing (98)
.editorconfig(1 hunks)Nhl.Api.Common/Exceptions/InvalidPlayerPositionException.cs(1 hunks)Nhl.Api.Common/Exceptions/InvalidSeasonException.cs(1 hunks)Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs(1 hunks)Nhl.Api.Common/Exceptions/NhlApiRequestException.cs(1 hunks)Nhl.Api.Common/Extensions/EnumExtensions.cs(1 hunks)Nhl.Api.Common/Extensions/StringExtensions.cs(1 hunks)Nhl.Api.Common/Helpers/CountryCodeHelper.cs(1 hunks)Nhl.Api.Common/Http/NhlApiHttpClient.cs(5 hunks)Nhl.Api.Common/Http/NhlApiWebHttpClient.cs(2 hunks)Nhl.Api.Common/Http/NhlCmsHttpClient.cs(2 hunks)Nhl.Api.Common/Http/NhlEApiHttpClient.cs(2 hunks)Nhl.Api.Common/Http/NhlScoresHtmlReportsApiHttpClient.cs(2 hunks)Nhl.Api.Common/Http/NhlShiftChartHttpClient.cs(2 hunks)Nhl.Api.Common/Http/NhlStaticAssetsApiHttpClient.cs(2 hunks)Nhl.Api.Common/Http/NhlSuggestionApiHttpClient.cs(2 hunks)Nhl.Api.Common/Nhl.Api.Common.csproj(1 hunks)Nhl.Api.Common/Services/CachingService.cs(3 hunks)Nhl.Api.Common/Services/NhlApiAsyncHelper.cs(3 hunks)Nhl.Api.Domain/Enumerations/Player/PlayerEnumFileGeneratorHelper.cs(1 hunks)Nhl.Api.Domain/Models/Award/Award.cs(0 hunks)Nhl.Api.Domain/Models/Draft/PlayerDraftRanking.cs(1 hunks)Nhl.Api.Domain/Models/Franchise/Franchise.cs(0 hunks)Nhl.Api.Domain/Models/Game/Broadcast.cs(0 hunks)Nhl.Api.Domain/Models/Game/Format.cs(1 hunks)Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs(23 hunks)Nhl.Api.Domain/Models/Game/GameCenterLanding.cs(30 hunks)Nhl.Api.Domain/Models/Game/GameCenterPlayByPlay.cs(16 hunks)Nhl.Api.Domain/Models/Game/GameMetadata.cs(3 hunks)Nhl.Api.Domain/Models/Game/GameScore.cs(16 hunks)Nhl.Api.Domain/Models/Game/GameScoreboard.cs(10 hunks)Nhl.Api.Domain/Models/Game/GameWatchSource.cs(2 hunks)Nhl.Api.Domain/Models/Game/GoalieSeasonGameLog.cs(7 hunks)Nhl.Api.Domain/Models/Game/LeagueRecord.cs(1 hunks)Nhl.Api.Domain/Models/Game/PlayerSeasonGameLog.cs(8 hunks)Nhl.Api.Domain/Models/Game/TeamScoreboard.cs(14 hunks)Nhl.Api.Domain/Models/Game/TvScheduleBroadcast.cs(2 hunks)Nhl.Api.Domain/Models/League/AmateurLeague.cs(1 hunks)Nhl.Api.Domain/Models/League/LeagueMetadataInformation.cs(5 hunks)Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs(16 hunks)Nhl.Api.Domain/Models/Player/ExpressionPlayerFilterBuilder.cs(18 hunks)Nhl.Api.Domain/Models/Player/GoalieProfile.cs(27 hunks)Nhl.Api.Domain/Models/Player/LastName.cs(1 hunks)Nhl.Api.Domain/Models/Player/PlayerData.cs(2 hunks)Nhl.Api.Domain/Models/Player/PlayerProfile.cs(16 hunks)Nhl.Api.Domain/Models/Player/PlayerSearchResponse.cs(1 hunks)Nhl.Api.Domain/Models/Player/PlayerSearchResult.cs(3 hunks)Nhl.Api.Domain/Models/Player/PlayerShiftInformation.cs(6 hunks)Nhl.Api.Domain/Models/Player/PlayerSpotlight.cs(3 hunks)Nhl.Api.Domain/Models/Player/TeamProspects.cs(1 hunks)Nhl.Api.Domain/Models/Schedule/LeagueSchedule.cs(21 hunks)Nhl.Api.Domain/Models/Schedule/LeagueScheduleCalendar.cs(4 hunks)Nhl.Api.Domain/Models/Schedule/PlayoffSeriesSchedule.cs(1 hunks)Nhl.Api.Domain/Models/Schedule/TeamSchedule.cs(1 hunks)Nhl.Api.Domain/Models/Schedule/TeamSeasonSchedule.cs(11 hunks)Nhl.Api.Domain/Models/Schedule/TeamWeekSchedule.cs(1 hunks)Nhl.Api.Domain/Models/Season/LeagueStandingsSeasonInformation.cs(2 hunks)Nhl.Api.Domain/Models/Season/TeamStatisticsSeason.cs(2 hunks)Nhl.Api.Domain/Models/Standing/LeagueStanding.cs(11 hunks)Nhl.Api.Domain/Models/Standing/LeagueStandingType.cs(1 hunks)Nhl.Api.Domain/Models/Statistics/GoalieStatisticsFilterResult.cs(5 hunks)Nhl.Api.Domain/Models/Statistics/GoalieStatisticsLeaders.cs(1 hunks)Nhl.Api.Domain/Models/Statistics/PlayerRealtimeStatisticsFilterResult.cs(6 hunks)Nhl.Api.Domain/Models/Statistics/PlayerStatisticLeaders.cs(3 hunks)Nhl.Api.Domain/Models/Statistics/PlayerStatisticsFilterResult.cs(5 hunks)Nhl.Api.Domain/Models/Statistics/PlayerTimeOnIceStatisticsFilterResult.cs(4 hunks)Nhl.Api.Domain/Models/Statistics/TeamStatisticsDetails.cs(0 hunks)Nhl.Api.Domain/Models/Team/CommonName.cs(1 hunks)Nhl.Api.Domain/Models/Team/TeamColors.cs(1 hunks)Nhl.Api.Domain/Models/Team/TeamLogo.cs(1 hunks)Nhl.Api.Domain/Models/Team/TeamName.cs(1 hunks)Nhl.Api.Domain/Models/Team/TeamSeasonRoster.cs(6 hunks)Nhl.Api.Domain/Models/Team/TeamSeasonStatistics.cs(4 hunks)Nhl.Api.Domain/Models/Team/Teams.cs(1 hunks)Nhl.Api.Domain/Nhl.Api.Domain.csproj(1 hunks)Nhl.Api.Domain/Services/NhlTeamService.cs(1 hunks)Nhl.Api.Tests/CachingServiceTests.cs(3 hunks)Nhl.Api.Tests/DateTimeHelperTests.cs(2 hunks)Nhl.Api.Tests/GameTests.cs(4 hunks)Nhl.Api.Tests/Helpers/Attributes/TestMethodWithRetryAttribute.cs(1 hunks)Nhl.Api.Tests/LeagueTests.cs(8 hunks)Nhl.Api.Tests/LinqExtensionTests.cs(2 hunks)Nhl.Api.Tests/Nhl.Api.Tests.csproj(1 hunks)Nhl.Api.Tests/NhlApiAsyncHelperTests.cs(1 hunks)Nhl.Api.Tests/PlayerTests.cs(11 hunks)Nhl.Api.Tests/ProjectStructureTests.cs(7 hunks)Nhl.Api.Tests/StatisticsTests.cs(10 hunks)Nhl.Api.Tests/StringTests.cs(2 hunks)Nhl.Api.Tests/TeamTests.cs(5 hunks)Nhl.Api/.editorconfig(1 hunks)Nhl.Api/Nhl.Api.csproj(1 hunks)Nhl.Api/README.md(4 hunks)Nhl.Api/Src/Api/NhlApi.cs(1 hunks)Nhl.Api/Src/GameApi/NhlGameApi.cs(0 hunks)Nhl.Api/Src/LeagueApi/INhlLeagueApi.cs(2 hunks)Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs(11 hunks)Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs(5 hunks)Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs(11 hunks)
💤 Files with no reviewable changes (5)
- Nhl.Api.Domain/Models/Game/Broadcast.cs
- Nhl.Api.Domain/Models/Award/Award.cs
- Nhl.Api.Domain/Models/Franchise/Franchise.cs
- Nhl.Api/Src/GameApi/NhlGameApi.cs
- Nhl.Api.Domain/Models/Statistics/TeamStatisticsDetails.cs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs
[warning] 8-8: Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs#L8
Added line #L8 was not covered by tests
[warning] 13-13: Nhl.Api.Common/Exceptions/InvalidTeamAbbreviationException.cs#L13
Added line #L13 was not covered by tests
Nhl.Api.Common/Exceptions/InvalidSeasonException.cs
[warning] 10-10: Nhl.Api.Common/Exceptions/InvalidSeasonException.cs#L10
Added line #L10 was not covered by tests
Nhl.Api.Common/Exceptions/InvalidPlayerPositionException.cs
[warning] 10-10: Nhl.Api.Common/Exceptions/InvalidPlayerPositionException.cs#L10
Added line #L10 was not covered by tests
Nhl.Api.Common/Exceptions/NhlApiRequestException.cs
[warning] 10-10: Nhl.Api.Common/Exceptions/NhlApiRequestException.cs#L10
Added line #L10 was not covered by tests
Nhl.Api.Domain/Models/Game/Format.cs
[warning] 14-14: Nhl.Api.Domain/Models/Game/Format.cs#L14
Added line #L14 was not covered by tests
[warning] 21-21: Nhl.Api.Domain/Models/Game/Format.cs#L21
Added line #L21 was not covered by tests
Nhl.Api.Common/Helpers/CountryCodeHelper.cs
[warning] 1250-1250: Nhl.Api.Common/Helpers/CountryCodeHelper.cs#L1250
Added line #L1250 was not covered by tests
Nhl.Api.Common/Http/NhlCmsHttpClient.cs
[warning] 11-11: Nhl.Api.Common/Http/NhlCmsHttpClient.cs#L11
Added line #L11 was not covered by tests
[warning] 37-38: Nhl.Api.Common/Http/NhlCmsHttpClient.cs#L37-L38
Added lines #L37 - L38 were not covered by tests
Nhl.Api.Common/Http/NhlApiHttpClient.cs
[warning] 65-65: Nhl.Api.Common/Http/NhlApiHttpClient.cs#L65
Added line #L65 was not covered by tests
[warning] 106-106: Nhl.Api.Common/Http/NhlApiHttpClient.cs#L106
Added line #L106 was not covered by tests
Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs
[warning] 23-23: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L23
Added line #L23 was not covered by tests
[warning] 58-58: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L58
Added line #L58 was not covered by tests
[warning] 110-110: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L110
Added line #L110 was not covered by tests
[warning] 116-116: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L116
Added line #L116 was not covered by tests
[warning] 317-317: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L317
Added line #L317 was not covered by tests
[warning] 338-338: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L338
Added line #L338 was not covered by tests
[warning] 345-345: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L345
Added line #L345 was not covered by tests
[warning] 470-470: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L470
Added line #L470 was not covered by tests
[warning] 491-491: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L491
Added line #L491 was not covered by tests
[warning] 498-498: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L498
Added line #L498 was not covered by tests
[warning] 510-510: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L510
Added line #L510 was not covered by tests
[warning] 516-516: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L516
Added line #L516 was not covered by tests
[warning] 522-522: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L522
Added line #L522 was not covered by tests
[warning] 528-528: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L528
Added line #L528 was not covered by tests
[warning] 737-737: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L737
Added line #L737 was not covered by tests
[warning] 772-772: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L772
Added line #L772 was not covered by tests
[warning] 824-824: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L824
Added line #L824 was not covered by tests
[warning] 830-830: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L830
Added line #L830 was not covered by tests
🪛 markdownlint-cli2 (0.17.2)
Nhl.Api/README.md
1058-1058: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
1070-1070: Link fragments should be valid
null
(MD051, link-fragments)
2222-2222: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
2234-2234: Link fragments should be valid
null
(MD051, link-fragments)
🔇 Additional comments (209)
Nhl.Api.Domain/Nhl.Api.Domain.csproj (2)
4-4: Version Update ConfirmedThe
<Version>element has been updated to 3.7.0, which aligns with the PR release strategy. Ensure that any consuming projects or package dependencies are updated accordingly.
5-5: Target Framework Upgrade NotedThe
<TargetFrameworks>element has been updated to net9.0. This should allow you to take advantage of the latest features in .NET 9.0. Please verify that all project dependencies and CI/CD pipelines are compatible with this new framework version.Nhl.Api.Domain/Services/NhlTeamService.cs (2)
217-221: Improved robustness with null/whitespace checkThe changes add a null/whitespace check before adding team codes to the collection, preventing empty or null values from being included in the result. This improves the robustness of the method and helps avoid potential downstream issues when working with the returned list.
I also notice the use of the discard operator (
_) with theAddmethod, which is a good practice to explicitly show that the return value is intentionally ignored.
224-224: Modern syntax using collection expressionsThe return statement has been updated to use C# collection expressions (
[.. teamCodes]) instead of the previousteamCodes.ToList(). This leverages newer C# language features and provides a more concise way to convert the HashSet to a List.This change aligns with the PR's update to .NET 9.0 and modern C# capabilities.
Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (12)
2-2: Appropriate inclusion of exceptions namespace.The addition of the Exceptions namespace import is consistent with the error handling improvements made throughout the file.
300-301: Good performance optimization for string comparison.Changing from
StringComparison.InvariantCultureIgnoreCasetoStringComparison.OrdinalIgnoreCaseis an appropriate optimization for comparing league abbreviations, which don't require culture-specific handling.OrdinalIgnoreCaseis more performant for simple case-insensitive string comparisons.
307-312: Appropriate defensive programming.Adding this null check for team abbreviation improves error handling by gracefully returning empty results instead of potentially causing exceptions later in the execution flow.
397-397: Consistent string comparison improvement.Good consistency in applying the same
StringComparison.OrdinalIgnoreCaseoptimization here as was done earlier in the file.
405-410: Good error handling enhancement.This null check for team abbreviation follows the same pattern as earlier in the file, maintaining consistency in error handling throughout the API.
439-439: Improved code conciseness.The simplified lambda expression in the ForEach method improves readability while maintaining the same functionality.
471-472: Modern C# syntax improvement.Using
Enum.GetValues<TeamEnum>()is a cleaner, more modern approach to retrieving all enum values compared to the previous implementation.
473-492: Excellent resilience enhancement for team roster retrieval.Adding a try-catch block to handle cases where teams don't have a roster for a specific season (with well-documented examples) significantly improves the robustness of the API. The code now gracefully handles edge cases like teams that didn't exist in certain seasons rather than throwing exceptions.
495-495: Modern C# collection initialization.Using collection initializers with
[]syntax for empty collections is a good adoption of modern C# language features.
527-533: Appropriate exception handling.Good addition of specific exception with clear error message when team abbreviation is invalid, which will help with troubleshooting and debugging.
550-550: Improved code conciseness.The simplified lambda expression improves readability while maintaining the same functionality.
597-597: Good practice for string formatting.Using
_ = endpoint.Append(CultureInfo.InvariantCulture, $"&{expressionPlayerFilter}")ensures consistent string formatting across cultures while acknowledging that the return value isn't needed.Also applies to: 640-640, 683-683, 726-726
Nhl.Api.Tests/ProjectStructureTests.cs (7)
1-1: Appropriate addition of System.Globalization namespace.This namespace is correctly added to support the use of
CultureInfo.InvariantCulturein line 92 for culture-invariant string formatting.
15-15: Good improvement with readonly modifier and collection expressions.Adding the
readonlymodifier improves code safety by ensuring the array reference can't be changed after initialization. The code also uses the newer C# collection expression syntax[...]instead of the traditional array initialization.
53-53: Removedthisqualifier for better readability.Removing the
this.qualifier fromRootDirectoryFolderaccess improves code readability, which is good since the property is now static as shown in line 102.
65-65: Good use of discard pattern for StringBuilder.Using
_ = errors.Append(...)explicitly shows that the return value ofStringBuilder.Append()is intentionally ignored, which improves code clarity.
92-92: Improved string formatting with culture specification.Adding
CultureInfo.InvariantCulturetoAppendLineensures culture-invariant string formatting, which is a good practice for consistent output across different locales.
102-102: Appropriate use of static modifier.Changing
RootDirectoryFolderfrom an instance property to a static property is appropriate since it doesn't use instance state. This better expresses the property's nature and allows it to be accessed without an instance of the class.
24-24:❓ Verification inconclusive
Verify if discard pattern is necessary.
The discard pattern
_ =is used to explicitly ignore the return value ofMSBuildLocator.RegisterDefaults(). This is a good practice for clarity, but verify if this method actually returns a value that needs to be ignored.
🏁 Script executed:
#!/bin/bash # Check if MSBuildLocator.RegisterDefaults() returns a value rg -A 3 -B 3 "public static \w+ RegisterDefaults" --type csLength of output: 61
Important: Verify the Return Type of MSBuildLocator.RegisterDefaults()
- Our initial search in the repository did not reveal a definition for
RegisterDefaults(), suggesting it might be part of an external dependency.- Since we couldn’t confirm whether the method returns a value (or if that value is meaningful), please manually verify the method’s documentation or source (if available) to confirm whether ignoring the return value via the discard pattern is necessary.
- If the method returns void (or a value that is safe to ignore), consider removing the discard assignment; otherwise, retaining it for clarity is acceptable.
Nhl.Api/.editorconfig (1)
446-447: LGTM! Suppressing IDE1006 naming style warningsDisabling IDE1006 warnings is appropriate if you need to use naming styles that don't align with standard conventions, especially in an API that might need to match specific NHL naming patterns.
Nhl.Api.Common/Services/NhlApiAsyncHelper.cs (2)
13-14: LGTM! Code formatting improvementThe constructor parameter placement is now more readable with parameters on separate lines.
61-61: LGTM! Explicit use of discard operator for semaphore.Release()Using the discard operator (
_=) explicitly indicates that the return value is intentionally ignored, which improves code clarity.Nhl.Api.Common/Exceptions/InvalidPlayerPositionException.cs (1)
10-10: LGTM! Clean primary constructor syntax with proper base initializationThe change to primary constructor syntax with proper base class initialization is a good modernization that makes the code more concise.
You might want to add test coverage for this updated constructor as static analysis indicates it's not covered by tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: Nhl.Api.Common/Exceptions/InvalidPlayerPositionException.cs#L10
Added line #L10 was not covered by tests.editorconfig (1)
446-450: Configuration update to allow underscores in identifiers.Disabling the CA1707 and IDE1006 warnings removes restrictions on using underscores in identifiers. This change provides more flexibility in naming conventions, which can be particularly useful for unit tests or private fields following certain patterns.
Nhl.Api.Common/Extensions/StringExtensions.cs (1)
17-17: Improved readability withvarkeyword.Converting explicit type declarations to
varmakes the code more concise while maintaining readability, as the types are still clear from the initialization expressions. This change follows modern C# coding practices.Also applies to: 20-23
Nhl.Api.Domain/Models/Team/CommonName.cs (1)
14-14: Good enhancement to property type safetyAdding the
requiredmodifier to theDefaultproperty ensures this essential team name field is always initialized during object creation, preventing potential null reference exceptions.Note that this is a breaking change for any code that currently creates
CommonNameobjects without initializing theDefaultproperty, but it's a worthwhile improvement for type safety.Nhl.Api.Domain/Models/Game/LeagueRecord.cs (1)
32-32: Good enhancement to property type safetyAdding the
requiredmodifier to theTypeproperty ensures that this field is always initialized during object creation, which aligns with its description as an essential record type identifier.This change improves nullability handling and prevents potential null reference exceptions at runtime.
Nhl.Api.Common/Nhl.Api.Common.csproj (2)
4-4: Version increment to 3.7.0 looks appropriateThe version bump from 3.6.1 to 3.7.0 follows semantic versioning conventions, indicating new functionality (the
requiredproperty modifiers) while maintaining backward compatibility at the API level.
5-5:✅ Verification successful
Verify .NET 9.0 target readiness
Upgrading to .NET 9.0 is a significant change. As of March 2025, ensure that:
- .NET 9.0 is officially released or in an appropriate preview stage for your project
- Your CI pipeline supports building with .NET 9.0
- Your target deployment environments support .NET 9.0
This change will require users of your library to have .NET 9.0 installed.
🌐 Web query:
What is the current release status of .NET 9.0?💡 Result:
.NET 9.0, released in November 2024, is currently an active Standard Term Support (STS) release, with the latest patch version 9.0.2 issued on February 11, 2025[1][3][8][9]. Here's a detailed breakdown of its release status and features:
Release Timeline and Support
- General Availability: November 12, 2024[3][8].
- Latest Patch: 9.0.2 (February 11, 2025), addressing security and performance fixes[1][9].
- Support End Date: May 12, 2026 (18 months of STS support)[3][8].
Key Features and Improvements
Performance Enhancements:
- Optimized garbage collection with dynamic adaptation to application size[2].
- Loop optimizations and Arm64 vectorization in the runtime[2].
- ASP.NET Core improvements for static file handling (automatic compression, fingerprinted versioning)[5][7].
Security Fixes:
- Critical vulnerabilities resolved in patches (e.g., CVE-2025-21171, CVE-2025-21173), including remote code execution and privilege escalation risks[1][7].
New APIs and Libraries:
CountByandAggregateBymethods in LINQ for efficient state aggregation[2].PersistedAssemblyBuilderfor saving emitted assemblies with PDB support[2][6].- KMAC algorithm support in cryptography[2][5].
SDK and Tooling:
- Introduction of workload sets for version-pinned toolchains[2].
- Enhanced terminal logger and MSBuild script analyzers[2][4].
dotnet workload historyto track workload changes[2].Update Guidance
- Visual Studio: Requires version 17.12+ for .NET 9 compatibility[4].
- Project Migration: Update
TargetFrameworktonet9.0and upgrade NuGet packages (e.g., EF Core, ASP.NET Core)[4][7].- Security Audits: NuGet now audits both direct and transitive dependencies by default[2][7].
Support Policy
- STS vs. LTS: .NET 9 is an STS release, while .NET 8 (LTS) remains supported until November 2026[3][8].
- Patch Cadence: Monthly updates on "Patch Tuesday," cumulative for security and non-security fixes[3].
For developers, upgrading to .NET 9.0.2 is recommended to leverage performance gains and critical security patches, particularly for cloud-native and high-throughput applications[1][2][5].
Citations:
- 1: https://github.com/dotnet/core/blob/main/release-notes/9.0/9.0.1/9.0.1.md
- 2: https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-9/overview
- 3: https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core
- 4: https://www.roundthecode.com/dotnet-blog/dotnet-9-released-how-do-you-update
- 5: https://devoxsoftware.com/blog/what-s-new-in-net-9-trends-and-features-for-2025/
- 6: https://visualstudiomagazine.com/Articles/2024/04/16/net-9-preview-3.aspx
- 7: https://www.softacom.com/wiki/development/net-in-2025-reasons-why-it-is-the-most-demanded-and-future-of-net/
- 8: https://endoflife.date/dotnet
- 9: https://versionsof.net
- 10: https://github.com/dotnet/core/blob/main/release-notes/9.0/preview/preview1/README.md
- 11: https://dotnet.microsoft.com/en-us/download/dotnet/9.0
- 12: https://github.com/dotnet/core/blob/main/release-notes/9.0/9.0.0/README.md
- 13: https://blog.inedo.com/dotnet/demystifying-lts/
- 14: https://en.wikipedia.org/wiki/.NET
- 15: .NET February 2025 Update - .NET 9.0.2 and .NET 8.0.13 dotnet/core#9726
.NET 9.0 Release Confirmed – Proceed with Upgrade
We've verified that .NET 9.0 is officially released (GA in November 2024) and is now an active STS release with the latest patch 9.0.2 (February 2025). In light of this confirmation, please ensure the following:
- Your CI pipeline is configured to build with .NET 9.0.
- Target deployment environments fully support .NET 9.0.
- Documentation clearly notes that library users must install .NET 9.0.
Nhl.Api.Domain/Models/Player/PlayerSearchResponse.cs (1)
15-15: Good improvement to obsolete classAdding the
requiredmodifier to theSuggestionsproperty improves type safety by ensuring this collection is always initialized.Since this class is marked as obsolete, the breaking change impact should be minimal as clients should be migrating to the recommended
PlayerSearchResultclass.Nhl.Api.Tests/Helpers/Attributes/TestMethodWithRetryAttribute.cs (1)
56-57: Improved control flow in test retry logicReplacing the return statement with a break statement enhances the method's control flow. This change ensures the method always exits through the final return statement at line 68, creating a more consistent flow and preventing potential null returns in edge cases where exceptions might occur. The break statement correctly exits the retry loop when tests pass, while still allowing the method to complete normally.
Nhl.Api.Tests/DateTimeHelperTests.cs (1)
1-1: Great addition of culture-invariant date handling!Using
CultureInfo.InvariantCulturefor date parsing and string formatting ensures consistent behavior across different regional settings, making the tests more robust and reliable.Also applies to: 48-48, 54-54
Nhl.Api.Tests/LinqExtensionTests.cs (2)
7-7: Good use ofinternal sealedmodifiers for the test class.Restricting the class to internal assembly access and preventing inheritance with
sealedfollows best practices for classes that don't need to be exposed or extended.
17-20: Nice modernization with target-typed new expressions.Using the concise
new()syntax improves readability while the type is still clearly inferred from the context. This is a good adoption of modern C# features.Nhl.Api/Src/LeagueApi/INhlLeagueApi.cs (1)
212-218: Well-documented addition of playoff series functionality.The new
GetPlayoffSeriesBySeasonYearAsyncmethod enhances the API with playoff schedule retrieval capabilities. The method is properly documented with XML comments and follows the established pattern of other API methods including proper parameter validation and cancellation support.Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (3)
1-1: Good addition of culture support.Adding the
System.Globalizationnamespace import prepares the class for culture-invariant operations, which is important for a library that may be used in various international contexts.
38-38: Nice use of modern C# collection expressions.Replacing
new List<PlayerSearchResult>()with[]uses C# 12's collection expressions feature for a more concise and readable syntax when returning empty collections.Also applies to: 55-55
75-76: Improved method clarity and culture-invariant parsing.The addition of
this.prefix to method calls clarifies that instance methods are being called. More importantly, usingCultureInfo.InvariantCulturewithint.Parse()ensures consistent numeric parsing regardless of the user's regional settings, making the code more robust for international use.Also applies to: 101-102
Nhl.Api.Tests/TeamTests.cs (5)
1-1: Added culture support for date parsingAdded the
System.Globalizationnamespace to support culture-invariant date parsing, which is used later in the file to ensure consistent date parsing behavior regardless of the system's regional settings.
68-68: Updated to use modern generic Enum.GetValues syntaxReplaced
Enum.GetValues(typeof(TeamEnum))with the more modern generic syntaxEnum.GetValues<TeamEnum>(), which improves type safety and readability.
236-236: Updated test data to use newer seasonTest case updated to use the more recent
SeasonYear.season20242025value, ensuring tests are up-to-date with the latest available NHL seasons.
431-431: Added culture-invariant date parsingAdded
CultureInfo.InvariantCultureto theDateOnly.Parsemethod to ensure consistent date parsing behavior regardless of the system's regional settings. This makes tests more reliable across different environments.
452-452: Added culture-invariant date parsingAdded
CultureInfo.InvariantCultureto theDateOnly.Parsemethod to ensure consistent date parsing behavior regardless of the system's regional settings. This makes tests more reliable across different environments.Nhl.Api.Tests/GameTests.cs (4)
1-1: Added culture support for date parsingAdded the
System.Globalizationnamespace to support culture-invariant date parsing, which is used later in the file to ensure consistent date parsing behavior regardless of the system's regional settings.
46-46: Changed collection emptiness check approachModified the assertion from using
results.Any()to a direct property check withresults.Count != 0. Both achieve the same result, but the property access is more direct and potentially slightly more efficient.
166-166: Added culture-invariant date parsingAdded
CultureInfo.InvariantCultureto theDateOnly.Parsemethod to ensure consistent date parsing behavior regardless of the system's regional settings. This makes tests more reliable across different environments.
187-187: Changed collection emptiness check approachModified the assertion from using
results.Any()to a direct property check withresults.Count != 0. Both achieve the same result, but the property access is more direct and potentially slightly more efficient.Nhl.Api.Domain/Models/Team/Teams.cs (2)
14-14: Enhanced type safety with required modifierAdded the
requiredmodifier to theAwayTeamproperty, ensuring it must be initialized during object creation. This improves type safety by preventing potential null reference exceptions and making initialization requirements explicit.
20-20: Enhanced type safety with required modifierAdded the
requiredmodifier to theHomeTeamproperty, ensuring it must be initialized during object creation. This improves type safety by preventing potential null reference exceptions and making initialization requirements explicit.Nhl.Api.Tests/NhlApiAsyncHelperTests.cs (2)
15-15: Excellent use of modern C# syntax.The update to use C# 12's collection expression syntax
[...]instead ofnew[]makes the code more concise and readable.
20-24:❓ Verification inconclusive
Consider the implications of the threading model change.
The removal of the generic type parameter
<int>suggests theForEachAsyncmethod has been updated to use type inference. Additionally, wrapping the operation inTask.Run()changes the execution context to run on a separate thread from the thread pool rather than inline.While this may better simulate real-world asynchronous operations, ensure this doesn't introduce test flakiness in CI environments where thread pool saturation could occur.
🏁 Script executed:
#!/bin/bash # Check implementation of ForEachAsync to understand the changes in threading model rg -A 20 "public static.*ForEachAsync" --type csLength of output: 1578
Attention: Review the asynchronous execution context
The updated test now relies on type inference (omitting the explicit
<int>), which is expected given the refactoredForEachAsync<T>implementation using a semaphore for concurrency control. Also, wrapping the operation inTask.Run()causes the lambda to execute on a separate thread, which better simulates real-world async behavior. However, please double-check that this additional context switch won’t introduce any CI flakiness, particularly under conditions where the thread pool might become saturated.
- Verify that the CI environment can handle the thread pool usage when multiple tests run concurrently.
- Confirm that the behavior change introduced by
Task.Run()is intentional and aligns with the test’s purpose.Nhl.Api.Domain/Models/Season/TeamStatisticsSeason.cs (2)
26-26: Improved type safety with required modifier.Adding the
requiredmodifier to theTeamNameproperty enforces that it must be initialized during object creation, which prevents null reference exceptions at runtime by moving these checks to compile time.
38-38: Modern C# syntax for collection initialization.The change from
new()to[]uses C# 12's collection expressions for a more concise and readable initialization of theGameTypeslist.Nhl.Api.Domain/Models/Team/TeamName.cs (1)
13-13: Improved type safety with required modifiers for language-specific team names.Adding the
requiredmodifier to bothDefault(English) andFr(French) properties ensures that these essential team name translations must be provided during object initialization. This is a good enhancement to prevent null reference exceptions and ensures data integrity.Also applies to: 19-19
Nhl.Api.Domain/Models/Standing/LeagueStandingType.cs (1)
14-14: Good use ofrequiredmodifier for enhanced type safetyAdding the
requiredmodifier to theNameandDescriptionproperties ensures these essential fields are always initialized during object creation, preventing potential null reference exceptions and improving the class contract.Also applies to: 21-21
Nhl.Api.Common/Exceptions/InvalidSeasonException.cs (1)
7-10: Good use of modern C# features with primary constructorThe refactoring to use C# 12's primary constructor syntax makes the code more concise while maintaining the same functionality.
Static analysis indicates this line is not covered by tests. Consider adding a test case that explicitly validates this exception is thrown with the correct message when an invalid season is provided.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-10: Nhl.Api.Common/Exceptions/InvalidSeasonException.cs#L10
Added line #L10 was not covered by testsNhl.Api/Nhl.Api.csproj (1)
4-5: Version increment and framework updateThe version update to 3.7.0 and copyright year update are appropriate. However, upgrading to .NET 9.0 is significant as it requires users to adopt this framework version.
Ensure this framework update is prominently documented in release notes, as it may affect backward compatibility. Consider if maintaining a .NET 8.0 target in a multi-targeting approach would better serve users who haven't yet upgraded.
Also applies to: 8-8
Nhl.Api.Tests/CachingServiceTests.cs (1)
13-13: Style change to implicit typing withvarChanging from explicit tuple types to implicit typing with
varis a style preference that maintains the same functionality while making the code more concise.Also applies to: 27-27, 47-47
Nhl.Api.Common/Http/NhlScoresHtmlReportsApiHttpClient.cs (3)
3-3: Added necessary System.Threading import to support the Lock class.This import is required for the new Lock type being used instead of the previous object for thread synchronization.
11-11: Improved thread synchronization by replacing generic object with Lock class.Replacing the generic object with a dedicated Lock class enhances thread safety and makes the locking mechanism's purpose more explicit in the code.
41-42: Improved clarity by using 'this' qualifier for property access.Using 'this' with Client and Timeout properties clarifies the source of these properties and enhances code readability, especially in inheritance scenarios.
Nhl.Api.Common/Http/NhlSuggestionApiHttpClient.cs (3)
3-3: Added necessary System.Threading import to support the Lock class.This import is required for the new Lock type being used instead of the previous object for thread synchronization.
11-11: Improved thread synchronization by replacing generic object with Lock class.Replacing the generic object with a dedicated Lock class enhances thread safety and makes the locking mechanism's purpose more explicit in the code.
39-40: Improved clarity by using 'this' qualifier for property access.Using 'this' with Client and Timeout properties clarifies the source of these properties and enhances code readability, especially in inheritance scenarios.
Nhl.Api.Domain/Models/Draft/PlayerDraftRanking.cs (1)
21-21: Enhanced type safety with 'required' modifier for essential properties.Adding the 'required' modifier to the Name and ConsumerKey properties ensures these critical values are always initialized during object creation, preventing potential null reference exceptions and making the API more robust.
Also applies to: 27-27
Nhl.Api.Common/Helpers/CountryCodeHelper.cs (1)
15-1249: Added comprehensive ISO country code mapping.The addition of extensive country code mappings significantly improves the functionality of this helper method by providing coverage for a wide range of international country codes.
Nhl.Api.Tests/StringTests.cs (2)
116-116: Consistent use of var keyword enhances code style uniformityThe change from explicit
stringtype tovaraligns with modern C# coding style and matches the pattern used in the rest of the class methods.
130-130: Consistent use of var keyword enhances code style uniformityThe change from explicit
stringtype tovaraligns with modern C# coding style and maintains consistency with other similar declarations in the test class.Nhl.Api.Domain/Models/League/AmateurLeague.cs (2)
14-14: Improved type safety with required modifierAdding the
requiredmodifier to theNameproperty enhances type safety by ensuring this property must be initialized during object creation, preventing potential null reference issues.
21-21: Improved type safety with required modifierAdding the
requiredmodifier to theLinkproperty enhances type safety by ensuring this property must be initialized during object creation, preventing potential null reference issues.Nhl.Api.Common/Http/NhlShiftChartHttpClient.cs (2)
11-12: Enhanced thread safety with dedicated Lock typeThe changes from a generic
objectto a specializedLockclass for synchronization and making_httpClientnullable (HttpClient?) improve thread safety and null handling. These changes align with modern C# practices for thread-safe singleton patterns.
37-38: Improved readability with explicit property accessAdding
this.qualifier to property access makes the code more readable by explicitly showing that these are instance properties, not local variables or parameters.Nhl.Api.Common/Http/NhlApiWebHttpClient.cs (2)
12-15: Enhanced thread safety with dedicated Lock type and warning suppressionThe change from generic
objectto a specializedLockclass improves thread safety. The warning suppression directives appropriately handle the compiler warnings about non-nullable field initialization, acknowledging that the field is initialized in a controlled manner within the lock.
42-43: Improved readability with explicit property accessAdding
this.qualifier to property access makes the code more readable by explicitly showing that these are instance properties, not local variables or parameters. This change maintains consistency with the code style used in other HTTP client classes.Nhl.Api.Common/Http/NhlStaticAssetsApiHttpClient.cs (3)
3-3: Added System.Threading namespace for Lock usageThe System.Threading namespace has been added to support the new Lock type used for synchronization.
11-11: Changed lock object type from object to LockReplacing the generic object with a more specialized Lock type provides clearer intent for thread synchronization.
38-39: Added 'this' qualifier to property accessUsing explicit 'this' qualifiers improves code clarity by making it clear that instance properties are being accessed rather than local or static variables.
Nhl.Api.Common/Http/NhlCmsHttpClient.cs (4)
3-3: Added System.Threading namespace for Lock usageThe System.Threading namespace has been added to support the new Lock type used for synchronization.
12-12: Made HttpClient nullableMaking _httpClient nullable better represents that it might be null before initialization. This is a more type-safe approach than using pragma directives.
37-38: Added 'this' qualifier to property accessUsing explicit 'this' qualifiers improves code clarity by making it clear that instance properties are being accessed rather than local or static variables. Static analysis indicates these lines are not covered by tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-38: Nhl.Api.Common/Http/NhlCmsHttpClient.cs#L37-L38
Added lines #L37 - L38 were not covered by tests
11-11:❓ Verification inconclusive
Changed lock object type from object to Lock
Replacing the generic object with a more specialized Lock type provides clearer intent for thread synchronization. Static analysis indicates this line is not covered by tests.
🏁 Script executed:
#!/bin/bash # Check for test coverage of NhlCmsHttpClient # Find tests for NhlCmsHttpClient rg -l "NhlCmsHttpClient" --glob="*.cs" | grep -i testLength of output: 53
Thread Synchronization Improvement – Verify Test Coverage
- The lock in
Nhl.Api.Common/Http/NhlCmsHttpClient.cshas been updated to use a specializedLocktype instead of a generic object, which clearly communicates its intended purpose for thread synchronization.- However, our static analysis indicates that this change is not currently exercised by any tests. Please verify manually whether tests exist that cover
NhlCmsHttpClient(or consider adding tests to validate the synchronization behavior).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 11-11: Nhl.Api.Common/Http/NhlCmsHttpClient.cs#L11
Added line #L11 was not covered by testsNhl.Api.Domain/Models/Player/TeamProspects.cs (3)
15-15: Added 'required' modifier to Forwards propertyThe 'required' modifier enforces that this property must be initialized during object construction, providing compile-time validation instead of runtime errors for uninitialized properties.
21-21: Added 'required' modifier to Defensemen propertyThe 'required' modifier enforces that this property must be initialized during object construction, providing compile-time validation instead of runtime errors for uninitialized properties.
27-27: Added 'required' modifier to Goalies propertyThe 'required' modifier enforces that this property must be initialized during object construction, providing compile-time validation instead of runtime errors for uninitialized properties.
Nhl.Api.Common/Http/NhlEApiHttpClient.cs (4)
3-3: Added System.Threading namespace for Lock usageThe System.Threading namespace has been added to support the new Lock type used for synchronization.
11-11: Changed lock object type from object to LockReplacing the generic object with a more specialized Lock type provides clearer intent for thread synchronization.
41-42: Added 'this' qualifier to property accessUsing explicit 'this' qualifiers improves code clarity by making it clear that instance properties are being accessed rather than local or static variables.
24-28:❓ Verification inconclusive
Modified constructor to initialize HttpClient immediately
The constructor now initializes _httpClient using an expression-bodied syntax with null-coalescing assignment and immediately sets its properties. This ensures the client is available right after construction.
However, initializing _httpClient in both the constructor and the HttpClient property getter could lead to inconsistent behavior if the constructor initialization is bypassed or if _httpClient is reset elsewhere.
🏁 Script executed:
#!/bin/bash # Check for potential _httpClient resets or bypasses of constructor # Look for direct assignments to _httpClient rg -A 3 -B 3 "_httpClient\s*=" --glob="*.cs" # Check for initialization of NhlEApiHttpClient without using the parameterless constructor rg -A 3 -B 3 "new NhlEApiHttpClient\(" --glob="*.cs"Length of output: 101
Once the above script is executed and the result inspected, please provide the output. This will help verify if there's any lazy initialization or further manipulation of _httpClient that might lead to the inconsistent behavior described in the review comment.
🏁 Script executed:
#!/bin/bash # Search for the declaration of the HttpClient property in Nhl.Api.Common/Http/NhlEApiHttpClient.cs to check for additional initialization logic. rg -A 5 "public\s+HttpClient\s+HttpClient" Nhl.Api.Common/Http/NhlEApiHttpClient.csLength of output: 85
Please run the above script and provide the output to help determine if there's any lazy initialization or conflicting assignment of _httpClient in the file.
🏁 Script executed:
#!/bin/bash # Search for all references to _httpClient in Nhl.Api.Common/Http/NhlEApiHttpClient.cs to check for any property getter or lazy initialization. rg -A 10 -B 10 "_httpClient" Nhl.Api.Common/Http/NhlEApiHttpClient.csLength of output: 1739
Review the dual initialization of _httpClient in the constructor and property getter
The constructor now eagerly initializes _httpClient using expression-bodied syntax, while the property getter uses a lazy initialization pattern with a lock. Although our search didn’t reveal any additional resets or alternative initialization paths, this mixed approach may lead to inconsistent behavior if—for any reason—the constructor’s initialization is skipped or if _httpClient is reassigned elsewhere.
Recommendations:
- Consolidate the initialization: Decide on a single consistent initialization strategy. For example, if lazy initialization is preferred for thread safety, remove the eager initialization in the constructor.
- Ensure thread safety uniformly: If you retain eager initialization, consider synchronizing it (e.g., using a lock) to match the thread-safe pattern in the getter.
- Review usage: Confirm that the current usage of _httpClient across the class (and potentially the wider codebase) doesn’t rely on this dual pattern, to avoid any unexpected behavior.
Nhl.Api.Domain/Models/Schedule/TeamSchedule.cs (3)
29-29: Addedrequiredmodifier to improve type safetyThe
requiredmodifier enforces that theClubTimezoneproperty must be initialized during object creation, preventing null values and enhancing type safety.
36-36: Addedrequiredmodifier to improve type safetyThe
requiredmodifier enforces that theClubUTCOffsetproperty must be initialized during object creation, preventing null values and enhancing type safety.
42-42: Updated list initialization to use modern C# syntaxChanged from
new()to[]for initializing theGamesproperty, leveraging newer C# collection initialization syntax for more concise code.Nhl.Api/Src/Api/NhlApi.cs (2)
748-756: Well-implemented new method for playoff series retrievalThe new
GetPlayoffSeriesBySeasonYearAsyncmethod follows the established pattern in the API with proper documentation, parameter validation via delegation to the league API, and cancellation token support.
747-747:❓ Verification inconclusive
Review async resource cleanup strategy
The asynchronous disposal was changed from awaiting
_nhlPlayerApi?.Dispose()to awaitingGC.SuppressFinalize(this). Similar to the synchronous Dispose method, this change could affect resource cleanup.Run the following script to verify how other DisposeAsync implementations handle resource cleanup in the codebase:
🏁 Script executed:
#!/bin/bash # Search for other DisposeAsync implementations to understand the async resource cleanup pattern rg -A 3 -B 3 "public async ValueTask DisposeAsync\(\)" --type csharpLength of output: 577
Critical: Verify Async Resource Cleanup in DisposeAsync
The current implementation in
Nhl.Api/Src/Api/NhlApi.cs(line 747) wrapsGC.SuppressFinalize(this)in aTask.Runand omits the previously awaited_nhlPlayerApi?.Dispose(). This change means that any asynchronous disposal or cleanup logic for_nhlPlayerApi(or similar resources) is no longer executed, which could lead to resource leakage if such cleanup is required.
- Location:
Nhl.Api/Src/Api/NhlApi.cs, line 747- Concern: The asynchronous disposal now only suppresses finalization rather than disposing of dependent resources as done previously.
- Recommendation: Please verify whether
_nhlPlayerApi(and any other disposable resources) requires explicit asynchronous disposal. If so, consider reinstating its disposal inDisposeAsyncor ensure that it is handled elsewhere to maintain proper resource cleanup.Nhl.Api.Tests/LeagueTests.cs (10)
1-1: Added culture-specific namespace for date parsing improvementsAdded
System.Globalizationnamespace to support culture-invariant date parsing in tests.
28-28: Enhanced date parsing with culture invarianceImproved the robustness of date parsing by specifying
CultureInfo.InvariantCulture, ensuring tests produce consistent results regardless of regional settings.
199-199: Enhanced date parsing with culture invarianceImproved the robustness of date parsing by specifying
CultureInfo.InvariantCulture, ensuring tests produce consistent results regardless of regional settings.
242-242: Enhanced date parsing with culture invarianceImproved the robustness of date parsing by specifying
CultureInfo.InvariantCulture, ensuring tests produce consistent results regardless of regional settings.
246-246: Optimized collection count checkChanged from using LINQ extension method
Count()to the more efficientCountproperty, eliminating unnecessary enumeration.
522-522: Enhanced date parsing with culture invarianceImproved the robustness of date parsing by specifying
CultureInfo.InvariantCulture, ensuring tests produce consistent results regardless of regional settings.
541-541: Enhanced date parsing with culture invarianceImproved the robustness of date parsing by specifying
CultureInfo.InvariantCulture, ensuring tests produce consistent results regardless of regional settings.
560-560: Enhanced date parsing with culture invarianceImproved the robustness of date parsing by specifying
CultureInfo.InvariantCulture, ensuring tests produce consistent results regardless of regional settings.
628-646: Added comprehensive test for new playoff series functionalityGood test coverage for the newly added
GetPlayoffSeriesBySeasonYearAsyncmethod, using multiple season years as test cases to verify proper functionality.
647-655: Added test for invalid input validationGood practice to test error handling by verifying that invalid season year throws an ArgumentException. This ensures proper validation is implemented in the API.
Nhl.Api.Domain/Models/Schedule/PlayoffSeriesSchedule.cs (5)
1-46: Well-designed base class for seeded teamsThe
Seedabstract class provides a solid foundation for team representation in playoff scenarios, with appropriate properties and JSON serialization attributes. Therequiredmodifier on string properties ensures type safety and prevents null values.
47-60: Clean inheritance for specialized team representationsThe
BottomSeedandTopSeedclasses inherit appropriately from theSeedbase class, allowing for specialized treatment in serialization and business logic while leveraging the common properties.
61-87: Well-structured main PlayoffSeriesSchedule classThe
PlayoffSeriesScheduleclass effectively models the playoff structure with appropriate properties for season identification, current round tracking, and a collection of rounds. The modern collection initialization syntax[]is used consistently.
88-121: Comprehensive Round class with appropriate propertiesThe
Roundclass properly encapsulates round-specific information with clear properties for identification and a collection of series. The XML documentation provides helpful context and examples.
122-175: Detailed Series class with complete playoff matchup informationThe
Seriesclass thoroughly models a playoff matchup with properties for identification, team details, and series progress tracking. Therequiredmodifier on string properties and complex types ensures the integrity of the model.Nhl.Api.Common/Services/CachingService.cs (3)
28-28: Good update to use nullable return type.The return type change from
Task<T>toTask<T?>properly reflects that this method can return null, aligning with C# nullable reference type best practices and providing better type safety.
41-41: LGTM: Modern syntax for collection initialization.Updated to use more concise C# syntax for initializing the ConcurrentDictionary.
70-83: Improved null handling in TryGetAsync method.The implementation now has better null-handling with additional checks for:
- Ensuring the retrieved value is not null
- Verifying the string value is not null or whitespace before attempting deserialization
These changes make the method more robust against potential exceptions when dealing with null or empty cached values.
Nhl.Api.Domain/Models/Team/TeamLogo.cs (1)
31-31: Good use ofrequiredmodifier to enforce data integrity.Adding the
requiredmodifier to these essential properties (Uri,ImageAsByteArray, andImageAsBase64String) ensures they are properly initialized during object creation, preventing incompleteTeamLogoobjects from being created.Also applies to: 37-37, 43-43
Nhl.Api.Domain/Models/Statistics/PlayerTimeOnIceStatisticsFilterResult.cs (2)
14-14: Good addition ofrequiredmodifier to ensure collection initialization.Adding the
requiredmodifier to thePlayerTimeOnIceStatisticsResultsproperty ensures that the collection is properly initialized during object creation, preventing potential null reference exceptions.
51-51: Consistent application ofrequiredmodifier enhances type safety.Adding the
requiredmodifier to key string properties (LastName,PositionCode,ShootsCatches,SkaterFullName, andTeamAbbrevs) ensures these essential identifying fields are always initialized, improving the robustness of the statistics model.Also applies to: 75-75, 123-123, 129-129, 135-135
Nhl.Api.Domain/Models/Statistics/GoalieStatisticsLeaders.cs (1)
35-35: Updated to modern C# collection initialization syntax.The code now uses the more concise collection expression syntax (
[]) introduced in C# 12 to initialize empty lists, making the code cleaner and more modern.Also applies to: 41-41, 47-47, 53-53
Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (8)
50-50: Good addition of CultureInfo.InvariantCulture for date parsing.Using CultureInfo.InvariantCulture ensures consistent date parsing regardless of the user's regional settings, which is important for an API library.
72-73: Good use of CultureInfo.InvariantCulture for date parsing.Consistent application of culture-invariant date parsing across all date-related methods ensures reliability.
95-95: Good use of CultureInfo.InvariantCulture for date parsing.Applying the invariant culture to date parsing here completes the consistent improvement across all season check methods.
111-111: Improved error handling with specific exception type.Using InvalidTeamAbbreviationException instead of a generic Exception improves API usage by allowing consumers to catch specific exception types.
135-135: Improved error handling with specific exception type.Consistent use of the specific exception type across similar methods. Well done.
430-430: Good use of CultureInfo.InvariantCulture for string conversion.This ensures consistent string conversion regardless of the user's regional settings.
561-561: Good use of CultureInfo.InvariantCulture for string formatting.Consistent application of culture settings for string formatting is good practice.
615-615: Good approach to determining league activity.This method efficiently combines all the specific season activity checks.
Nhl.Api.Domain/Models/Player/PlayerData.cs (2)
29-29: Good use of the 'required' modifier for essential properties.Using the 'required' modifier for FirstName, FullName, LastName, and PositionCode ensures these critical properties are always initialized, improving data integrity.
Also applies to: 36-36, 43-43, 50-50
69-69: Good use of C# 12 collection expressions.Using the new collection expression syntax
[]for initializing an empty list is more concise thannew().Nhl.Api.Domain/Models/Season/LeagueStandingsSeasonInformation.cs (1)
14-14: Good use of the 'required' modifier for essential properties.Adding the 'required' modifier to CurrentDate, Seasons, StandingsEnd, and StandingsStart ensures these critical properties are always initialized, improving data integrity.
Also applies to: 20-20, 75-75, 82-82
Nhl.Api.Domain/Models/Team/TeamColors.cs (2)
12-12: Good use of the 'required' modifier for essential properties.Making PrimaryColor and SecondaryColor required enforces that every team must have at least these two colors defined, which improves data consistency.
Also applies to: 18-18
24-24: Good approach making optional colors nullable.Making TertiaryColor, QuaternaryColor, QuinaryColor, SenaryColor, and SeptenaryColor nullable properly reflects that not all teams have these additional colors defined.
Also applies to: 30-30, 36-36, 42-42, 48-48
Nhl.Api.Domain/Models/Schedule/TeamWeekSchedule.cs (2)
15-15: Good use of therequiredmodifier for string properties.Adding the
requiredmodifier to these string properties enhances type safety by ensuring these essential properties are always initialized during object construction. This helps prevent potential null reference exceptions at runtime.Also applies to: 22-22, 29-29, 36-36, 43-43
49-49: Collection initialization syntax updated to use modern C# syntax.Changing from
new()to[]for initializing the Games collection uses the more modern collection expression syntax introduced in C# 12, making the code more concise.Nhl.Api.Domain/Models/Game/GameMetadata.cs (2)
14-14: Good use of therequiredmodifier for reference type properties.Adding the
requiredmodifier to these reference type properties (Teams list and SeasonStates) ensures these critical properties are initialized during object construction, preventing potential null reference exceptions.Also applies to: 20-20
33-33: Enhancing type safety with required properties.Adding the
requiredmodifier to Date, Name, and Tricode properties ensures these essential identifiers are always provided, improving data integrity and preventing potential null reference errors.Also applies to: 59-59, 66-66
Nhl.Api.Domain/Models/Statistics/PlayerStatisticsFilterResult.cs (2)
14-14: Good use of therequiredmodifier for collection property.Adding the
requiredmodifier to the PlayerStatisticsResults collection ensures this essential data is always initialized, preventing potential null reference exceptions when accessing this property.
83-83: Improved type safety for player identifier properties.Adding the
requiredmodifier to these string identifier properties (LastName, PositionCode, ShootsCatches, SkaterFullName, TeamAbbreviations) enhances data integrity by ensuring these core player attributes are always provided.Also applies to: 132-132, 181-181, 195-195, 202-202
Nhl.Api.Tests/PlayerTests.cs (8)
2-2: Added System.Net.Http namespace import.Adding this namespace import is appropriate for supporting the HttpRequestException usage in the test methods that verify API error handling.
159-159: Improved exception testing pattern.Adding the discard operator (
_) to the exception assertion calls is a good practice that explicitly shows the intention to ignore the returned exception object, suppressing any compiler warnings about unused variables.Also applies to: 169-169, 209-209, 222-222, 361-361, 375-375, 391-391, 406-406, 623-623, 821-821, 943-943, 954-954
289-299: Good addition of test for invalid season year format.This new test properly verifies that the API method correctly rejects invalid season year formats with appropriate exceptions, enhancing the test coverage for edge cases.
409-428: Comprehensive test for goalie information retrieval.This new test thoroughly validates that the GetGoalieInformationAsync method returns correct and detailed information for a specific player (Marc-Andre Fleury), covering all essential goalie attributes.
430-438: Good test for player type validation.This test verifies that the API correctly rejects requests for goalie information when a non-goalie player ID is provided, ensuring proper type checking in the API.
916-923: Thorough resource disposal testing.These new tests ensure that both synchronous and asynchronous disposal methods work correctly, which is important for proper resource management.
Also applies to: 927-934
937-944: Good validation tests for draft ranking parameters.These tests verify that the API properly validates input parameters for the GetPlayerDraftRankingByYearAsync method, ensuring robust parameter checking.
Also applies to: 948-955
380-383: Improved test data organization for player IDs.Converting the test data to use explicit player IDs instead of enums in these methods makes the tests more direct and easier to understand.
Also applies to: 395-398
Nhl.Api.Common/Http/NhlApiHttpClient.cs (5)
19-19: Positive Change: Removed new() constraintRemoving the
new()constraint from theGetAsync<T>method is a good improvement as it allows the method to work with class types that don't have a parameterless constructor, making the API more flexible.
59-59: Modernized code with primary constructor syntaxGood use of C# 12's primary constructor syntax, which makes the code more concise while maintaining readability.
70-70: Cleaner property initializationUsing property initializers with constructor parameters is a cleaner approach than having assignment statements in a constructor body. This makes the code more maintainable.
Also applies to: 75-75, 80-80
126-127: Safe handling of nullable HttpClientGood use of the null-conditional operator (
?.) with theBaseAddressproperty to safely access the address. The null-forgiving operator (!) is appropriately used after checking for nulls through request construction.Also applies to: 142-143
95-107:❓ Verification inconclusive
Enhanced error handling in GetAsync method
The improved error handling is excellent - now checking for null HTTP responses, non-successful status codes, and empty content. However, one of the error handling paths is not covered by tests.
The static analysis shows line 106 (empty content response handling) is not covered by tests. Consider adding a test case:
🏁 Script executed:
#!/bin/bash # Look for existing test coverage of GetAsync rg -A 5 "GetAsync" --type csharp --glob "*Test*.cs"Length of output: 1574
Action: Add a Test for the Empty Content Response Handling
The enhanced error handling for null responses and non-success status codes is solid. However, as confirmed by the test search, there is no specific test covering the empty content scenario (line 106). Please add a dedicated unit test to validate the behavior when the HTTP response content is empty.
- File: Nhl.Api.Common/Http/NhlApiHttpClient.cs (line 106)
- Suggested Test: Validate that an empty string or whitespace in the response content throws a proper HttpRequestException.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-106: Nhl.Api.Common/Http/NhlApiHttpClient.cs#L106
Added line #L106 was not covered by testsNhl.Api.Domain/Models/Statistics/GoalieStatisticsFilterResult.cs (1)
14-14: Improved type safety with required propertiesAdding the
requiredmodifier to these properties is an excellent change. It ensures that these critical properties must be initialized during object construction, preventing potential null reference exceptions at runtime.Also applies to: 55-55, 83-83, 146-146, 167-167
Nhl.Api.Domain/Models/Statistics/PlayerRealtimeStatisticsFilterResult.cs (1)
14-14: Enhanced type safety with required propertiesExcellent use of the
requiredmodifier on these string and list properties. This enforces proper initialization at object creation time, which helps prevent null reference exceptions and improves the overall API contract.Also applies to: 99-99, 153-153, 165-165, 177-177, 195-195
Nhl.Api.Domain/Models/Player/PlayerSpotlight.cs (2)
14-14: Strengthened type safety with required propertiesAdding the
requiredmodifier to these properties is a positive change that enforces initialization during object construction. This is particularly important for properties like URLs and identifiers that should never be null.Also applies to: 35-35, 42-42, 49-49, 70-70, 77-77, 84-84
35-35:❓ Verification inconclusive
Verify initialization of complex required property
The
Nameproperty is now required and is of typePlayerSpotlightName, which itself has a required property. Make sure that code initializingPlayerSpotlightobjects properly initializes both levels of required properties.
🏁 Script executed:
#!/bin/bash # Search for places where PlayerSpotlight objects are created to verify proper initialization rg "new PlayerSpotlight" -A 10 -B 2 --type csharpLength of output: 51
ACTION: Verify PlayerSpotlight Instantiation
The
Nameproperty is now mandatory, and its nested required property inPlayerSpotlightNamemust be initialized. However, our search usingrg "new PlayerSpotlight"didn’t return any instantiation points. This could indicate that PlayerSpotlight objects are created via indirect means (for example, through factory methods or dependency injection) or that the search pattern missed some cases.
- Manual Verification Needed: Please review all code paths (both direct and indirect) that create or obtain
PlayerSpotlightinstances to ensure that theNameproperty and its nested required properties are properly initialized.- Consider Enhancements: If object construction is handled in multiple ways, consider adding unit tests or assertions to enforce the invariant on required properties for better maintainability.
Nhl.Api.Domain/Models/Game/GoalieSeasonGameLog.cs (1)
54-54: Good enhancement of type safety with required modifierAdding the
requiredmodifier to these properties ensures they must be initialized during object creation, preventing potentialnullreference exceptions at runtime. This is a positive change that improves code robustness and aligns with the modern C# type system.Also applies to: 86-86, 107-107, 114-114, 121-121, 142-142, 149-149, 163-163, 198-198, 212-212
Nhl.Api.Domain/Models/Team/TeamSeasonStatistics.cs (1)
24-24: LGTM - Consistent application of required modifierThe addition of the
requiredmodifier to these properties is consistent with the changes in other model classes and enforces that critical data is always initialized. This improves type safety and provides better compile-time guarantees.Also applies to: 31-31, 38-38, 170-170, 183-183, 189-189, 209-209, 216-216, 223-223, 230-230
Nhl.Api.Domain/Models/Game/TvScheduleBroadcast.cs (1)
37-37: Great enhancement to broadcast modelsThe addition of the
requiredmodifier to string properties in the broadcast models ensures that essential broadcast information is always provided. This is particularly important for data that might be displayed directly to users.Also applies to: 44-44, 51-51, 58-58, 65-65, 72-72, 85-85, 92-92, 99-99, 105-105
Nhl.Api.Domain/Models/Game/GameWatchSource.cs (1)
18-18: Improved type safety with required propertiesThe addition of the
requiredmodifier to these string properties strengthens the contract of theGameWatchSourceclass by ensuring that essential fields cannot be left uninitialized. This change reduces the risk of null reference exceptions at runtime and makes the API more robust.Also applies to: 32-32, 39-39, 46-46, 53-53, 60-60, 67-67, 74-74, 81-81, 88-88, 95-95, 102-102
Nhl.Api.Tests/StatisticsTests.cs (7)
297-299: Added important default case handlingThe addition of a default case in the switch statement that throws an ArgumentException for unrecognized player enums is a defensive programming improvement. This prevents silent failures and makes debugging easier by failing fast when unexpected enum values are encountered.
349-349: Improved type safety with generic enum handlingUsing the generic
Enum.GetValues<PlayerEnum>()method instead of the non-generic version enhances type safety and eliminates the need for type casting, making the code more robust and maintainable.
360-360: Explicit discard of unused resultUsing the discard operator (
_) with Parallel.ForEach explicitly indicates that the result is intentionally ignored, improving code clarity and addressing compiler warnings about unused results.
387-391: Enhanced test flexibility with parameterized testsConverting the test to use DataRow parameters for seasonYear and gameType improves test maintainability and makes it easier to add new test cases without duplicating code.
403-403: Adjusted assertion thresholdThe assertion threshold for result.Count has been lowered from 500 to 250. This change appears deliberate, likely reflecting a more realistic expectation for the test data.
413-413: Improved clarity with explicit result discardingUsing the discard operator (
_) with Assert.ThrowsExceptionAsync clearly communicates that the return value is intentionally ignored while still verifying that an exception is thrown.Also applies to: 1348-1348, 1366-1366, 1384-1384, 1401-1401, 1420-1420, 1437-1437, 1454-1454, 1472-1472, 1489-1489, 1506-1506, 1524-1524
1475-1583: Comprehensive input validation testingThe addition of these test methods significantly improves test coverage by validating error handling for various invalid inputs (empty season years, invalid limits, invalid offsets, etc.). This helps ensure the API behaves predictably when receiving bad data.
Nhl.Api.Domain/Models/Game/GameScoreboard.cs (1)
23-23: Enhanced type safety with required propertiesAdding the
requiredmodifier to these reference type properties ensures proper initialization and prevents null reference exceptions at runtime. This is particularly important for properties likeTvBroadcasts,AwayTeam,HomeTeam, andGamesByDatethat contain critical data for the scoreboard functionality.Also applies to: 30-30, 44-44, 79-79, 86-86, 92-92, 106-106, 113-113, 119-119, 126-126, 133-133, 139-139, 145-145, 152-152, 165-165, 172-172, 185-185, 191-191, 211-211, 218-218, 232-232, 246-246, 259-259
Nhl.Api.Domain/Models/Game/PlayerSeasonGameLog.cs (1)
24-24: Strengthened data integrity with required propertiesThe addition of the
requiredmodifier to these properties ensures that essential player game log data cannot be left uninitialized. This improves the robustness of the API by enforcing proper object construction and preventing null reference exceptions when accessing these properties.Also applies to: 31-31, 38-38, 59-59, 66-66, 142-142, 156-156, 169-169, 176-176, 183-183, 203-203, 254-254, 292-292
Nhl.Api.Domain/Models/Standing/LeagueStanding.cs (1)
15-15: Improved type safety with required modifierThe addition of the
requiredmodifier to these string and object properties ensures they must be initialized during object creation. This enhances type safety and eliminates potential null reference exceptions when working with these models, which is particularly important for JSON deserialization.This change aligns with modern C# best practices for null safety and improves the robustness of your domain models.
Also applies to: 21-21, 40-40, 53-53, 74-74, 95-95, 102-102, 123-123, 389-389, 536-536, 550-550, 557-557, 564-564, 571-571, 618-618, 630-630, 636-636
Nhl.Api.Domain/Models/Player/ExpressionPlayerFilterBuilder.cs (3)
2-2: LGTM: Added System.Globalization for culture-invariant operationsThe addition of the System.Globalization namespace is required for the CultureInfo.InvariantCulture usage in string formatting operations.
41-41: Improved string handling with culture-invariant formattingExcellent improvement by specifying CultureInfo.InvariantCulture for all string formatting operations. This ensures consistent behavior across different locales and environments, preventing potential formatting inconsistencies in filter expressions that could lead to unexpected query results.
Also applies to: 52-52, 63-63, 74-74, 85-85, 95-95, 105-105, 115-115, 125-125, 138-138, 142-142, 157-157, 161-161, 187-187, 191-191, 206-206, 211-211, 226-226, 230-230
240-240: Enhanced code clarity and modernized syntaxThe changes to use explicit
this.references and the modern primary constructor syntax forExpressionPlayerFilterimprove code clarity and follow contemporary C# patterns. This makes the code more maintainable and aligns with best practices.Also applies to: 246-246, 262-277
Nhl.Api.Domain/Models/League/LeagueMetadataInformation.cs (1)
22-22: Improved model reliability with required propertiesAdding the
requiredmodifier to these properties enhances the reliability of your domain models. This ensures that essential information like player names, teams, and identifiers is always present when these objects are instantiated, preventing potential null reference exceptions and improving data integrity throughout the application.Also applies to: 42-42, 49-49, 69-69, 76-76, 83-83, 89-89, 101-101, 107-107, 113-113, 126-126, 133-133
Nhl.Api.Domain/Models/Team/TeamSeasonRoster.cs (2)
15-15: Enhanced data integrity with required propertiesThe addition of the
requiredmodifier to these properties strengthens the domain model by ensuring critical data like player information, team rosters, and biographical details are always initialized. This helps prevent null reference exceptions and improves the overall robustness of the API.Also applies to: 21-21, 27-27, 40-40, 47-47, 60-60, 67-67, 87-87, 94-94, 101-101, 115-115, 122-122, 157-157, 163-163, 170-170, 177-177
182-182: Improved null safety in FullName propertyThe updated implementation of the
FullNameproperty now uses null-conditional operators (?.) when accessingFirstNameandLastNameproperties. This is a defensive programming approach that prevents null reference exceptions even if the required properties somehow end up being null at runtime.Nhl.Api.Domain/Models/Statistics/PlayerStatisticLeaders.cs (2)
17-17: Modern collection initialization is a great improvement.The change from
new()to the new collection expression syntax[]for initializing lists is cleaner and leverages C# 12's modern features.Also applies to: 23-23, 29-29, 35-35, 41-41, 47-47, 53-53, 59-59, 65-65
85-85: Good use of therequiredmodifier to enforce property initialization.Adding the
requiredkeyword to these properties ensures they must be initialized at object creation time, which improves type safety and prevents null reference exceptions. This change aligns with modern C# practices for enforcing null safety in initialization.Also applies to: 92-92, 106-106, 113-113, 120-120, 127-127, 134-134
Nhl.Api.Domain/Models/Player/PlayerSearchResult.cs (3)
23-23: Appropriate use ofrequiredmodifier for non-null properties.Adding the
requiredmodifier to these string properties ensures they are initialized during object creation, improving type safety and avoiding potential null reference issues. This is a good practice for maintaining data integrity in the model.Also applies to: 42-42, 49-49, 56-56, 63-63, 71-71, 78-78, 85-85, 106-106, 113-113, 127-127, 134-134, 141-141, 148-148, 155-155
29-29: Improved FirstName and LastName properties with nullable types.Changing these properties to nullable string types with expression-bodied members is appropriate since they depend on the
Nameproperty having a space character. The nullable types properly indicate that these properties might return null values if the name format is unexpected.Also applies to: 35-35
92-92: Simplified FullBirthCountry with expression-bodied member.Converting to an expression-bodied property improves code readability while maintaining the same functionality.
Nhl.Api.Domain/Models/Schedule/LeagueScheduleCalendar.cs (1)
15-15: Consistent application ofrequiredmodifier across model classes.Adding the
requiredmodifier to these properties ensures they must be initialized during object creation, which strengthens type safety. This change is applied consistently across multiple classes in the file, showing a systematic approach to improving the API's robustness.Also applies to: 21-21, 34-34, 41-41, 55-55, 62-62, 69-69, 76-76, 82-82, 108-108, 115-115, 122-122, 129-129, 136-136, 143-143
Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs (3)
2-2: Enhanced string formatting with culture-invariant handling.Adding
CultureInfo.InvariantCultureto the string formatting operations is an excellent improvement that ensures consistent handling of numeric values and dates regardless of the user's culture settings. This prevents potential issues when the application runs in different regional environments.Also applies to: 40-40, 51-51, 62-62, 115-115, 119-119, 134-134, 138-138, 153-153, 157-157, 172-172, 176-176, 191-191, 196-196, 211-211, 215-215
225-225: Concise code with expression-bodied members.Converting these methods to expression-bodied members improves code conciseness while maintaining the same functionality. This approach is more modern and aligns with current C# best practices.
Also applies to: 231-231, 256-256, 261-261
244-249: Improved class definition with primary constructor and XML documentation.The improvements in this section include:
- Better XML documentation
- Use of primary constructor syntax
- Readonly field initialization directly from the constructor parameter
These changes follow modern C# coding practices and make the code more concise and safer.
Nhl.Api.Domain/Models/Game/GameScore.cs (1)
23-23: Enhancing type safety withrequiredpropertiesThe addition of the
requiredmodifier to these properties helps prevent null reference exceptions by ensuring these properties are initialized during object construction. This change improves the robustness of the data model and makes the API contracts more explicit.Also applies to: 30-30, 51-51, 64-64, 120-120, 127-127, 133-133, 147-147, 154-154, 160-160, 167-167, 174-174, 188-188, 194-194, 200-200, 206-206, 220-220, 226-226, 232-232, 238-238, 245-245, 259-259, 272-272, 279-279, 305-305, 312-312, 326-326, 333-333, 340-340, 368-368, 402-402, 409-409, 430-430, 450-450, 457-457, 464-464, 471-471, 478-478, 485-485, 492-492, 505-505, 512-512, 519-519, 525-525, 531-531, 537-537
Nhl.Api.Domain/Models/Game/TeamScoreboard.cs (1)
23-23: Consistent application of type safety enhancementsThe
requiredmodifier has been systematically applied to non-primitive properties across all team scoreboard related classes. This ensures consistency with the changes in the other model files and reinforces the same type safety benefits throughout the domain.Also applies to: 30-30, 44-44, 85-85, 92-92, 98-98, 112-112, 119-119, 125-125, 132-132, 139-139, 145-145, 151-151, 171-171, 178-178, 191-191, 197-197, 217-217, 224-224, 238-238, 245-245, 258-258, 265-265, 285-285, 298-298, 312-312, 319-319, 326-326, 332-332, 352-352, 359-359, 366-366, 379-379
Nhl.Api.Domain/Models/Player/PlayerProfile.cs (3)
38-38: Improved null safety for player profile modelsAdding the
requiredmodifier to these player-related properties ensures that critical player information is always present when objects are instantiated, reducing the risk of null reference exceptions.Also applies to: 45-45, 52-52, 59-59, 66-66, 80-80, 87-87, 94-94, 148-148, 155-155, 162-162, 169-169, 185-185, 192-192, 212-212, 218-218, 224-224, 230-230, 236-236, 242-242, 248-248, 254-254, 260-260
265-265: Improved code clarity with explicitthisreferenceThe modification to the
FullNameproperty ensures that member access is explicitly qualified withthis, improving code readability and preventing potential shadowing issues.
368-368: Consistent required property pattern in nested classesThe
requiredmodifier has been consistently applied to all relevant properties in nested classes, demonstrating a thorough approach to ensuring type safety throughout the player profile hierarchy.Also applies to: 374-374, 470-470, 488-488, 494-494, 584-584, 604-604, 610-610, 800-800, 810-810, 816-816, 876-876, 882-882, 900-900, 924-924, 930-930, 1032-1032
Nhl.Api.Domain/Models/Game/GameCenterPlayByPlay.cs (3)
26-26: Enhanced type safety in game center play-by-play modelThe
requiredmodifier has been added to properties across the play-by-play classes, ensuring that essential information is always provided when instantiating these objects.Also applies to: 33-33, 54-54, 60-60, 108-108, 115-115, 122-122, 153-153, 216-216, 223-223, 244-244, 251-251, 272-272, 278-278, 310-310, 317-317, 324-324, 331-331, 338-338, 352-352, 365-365, 412-412, 418-418, 432-432, 439-439, 452-452, 459-459, 472-472, 478-478, 484-484, 490-490, 509-509
445-445: Modern C# collection initialization syntaxThe usage of the empty collection initializer syntax
= []takes advantage of C# 12 features, aligning with the library's upgrade to .NET 9.0. This provides a more concise way to initialize collections while ensuring they're never null.Also applies to: 496-496, 515-515
544-544: Consistent required property pattern for roster spotsThe
requiredmodifier has been consistently applied to theRosterSpotclass properties, maintaining the same type safety benefits across all related models.Also applies to: 551-551, 565-565, 572-572
Nhl.Api.Domain/Models/Schedule/TeamSeasonSchedule.cs (2)
30-43: Enhancement of type safety with required modifiers.The addition of the
requiredmodifier to properties likeClubTimezone,ClubUTCOffset, andGamesenforces proper initialization at object creation time, improving overall type safety. Using C# 12's collection expressions (= []) for theGamesproperty is also a nice modern touch.
63-421: Consistent application of the required modifier across model properties.The systematic addition of the
requiredmodifier to string and object properties across theTeamSeasonScheduleAwayTeam,TeamSeasonScheduleGame,TeamSeasonScheduleGameOutcome, andTeamSeasonScheduleHomeTeamclasses strengthens the type system by ensuring these properties are properly initialized.Nhl.Api.Domain/Models/Player/GoalieProfile.cs (2)
38-357: Improved type safety through required properties.The comprehensive addition of the
requiredmodifier to the properties in theGoalieProfileclass and its related classes enhances type safety by ensuring all essential properties are initialized during object creation. This change aligns with the broader effort to enforce stricter initialization requirements throughout the domain models.
634-1126: Consistent application of required keyword to supporting classes.The addition of the
requiredmodifier to properties in the supporting classes likeTeamName,FirstName,GoalieSeasonTotal, etc. maintains a consistent approach to type safety across the entire object graph.Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs (4)
128-192: Type safety improvements for Boxscore and TeamGameStatistics.Adding the
requiredmodifier to these properties ensures proper initialization and prevents null reference exceptions.
219-498: Consistent required modifiers for player classes.The addition of the
requiredmodifier to properties inGameCenterBoxScoreDefense,GameCenterBoxScoreForwardand other player-related classes ensures consistent type safety throughout the player object model.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 317-317: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L317
Added line #L317 was not covered by tests
[warning] 338-338: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L338
Added line #L338 was not covered by tests
[warning] 345-345: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L345
Added line #L345 was not covered by tests
[warning] 470-470: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L470
Added line #L470 was not covered by tests
[warning] 491-491: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L491
Added line #L491 was not covered by tests
[warning] 498-498: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L498
Added line #L498 was not covered by tests
510-984: Comprehensive type safety improvements across supporting classes.The systematic addition of the
requiredmodifier to properties in classes likeGameCenterBoxScoreGameInfo,GameReports,GameCenterBoxScoreHomeTeam, etc. ensures a consistent approach to type safety throughout the object model.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 510-510: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L510
Added line #L510 was not covered by tests
[warning] 516-516: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L516
Added line #L516 was not covered by tests
[warning] 522-522: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L522
Added line #L522 was not covered by tests
[warning] 528-528: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L528
Added line #L528 was not covered by tests
[warning] 737-737: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L737
Added line #L737 was not covered by tests
[warning] 772-772: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L772
Added line #L772 was not covered by tests
[warning] 824-824: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L824
Added line #L824 was not covered by tests
[warning] 830-830: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L830
Added line #L830 was not covered by tests
23-86:❓ Verification inconclusive
Enhanced type safety in GameCenterBoxScoreAwayTeam class.
The addition of the
requiredmodifier to properties likeName,Abbrev,PowerPlayConversion,Logo, etc. ensures these essential properties are always initialized.Note: Static analysis shows that several modified lines lack test coverage. Consider adding tests to ensure these properties are properly initialized in various scenarios.
🏁 Script executed:
#!/bin/bash # Check for existing tests that might cover these models rg -A 2 -B 2 "GameCenterBoxScoreAwayTeam" --glob="*.Tests.cs"Length of output: 210
Enhanced Type Safety Confirmed; Please Manually Verify Test Coverage
The changes in
Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs—notably adding therequiredmodifier to properties likeName,Abbrev,PowerPlayConversion, andLogo—improve the model’s safety by ensuring these fields are always initialized. However, our recent static analysis did not conclusively locate tests covering these modifications (the automated search yielded no files due to filtering issues).
- Action Item: Manually verify or add unit tests to cover initialization scenarios for the
GameCenterBoxScoreAwayTeammodel.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L23
Added line #L23 was not covered by tests
[warning] 58-58: Nhl.Api.Domain/Models/Game/GameCenterBoxScore.cs#L58
Added line #L58 was not covered by testsNhl.Api.Domain/Models/Schedule/LeagueSchedule.cs (4)
23-85: Enhanced type safety for AwayTeam properties.The addition of the
requiredmodifier to properties likePlaceName,Abbrev,LightLogo, etc., ensures these properties are always initialized. The change fromnew()to= []for theOddscollection also adopts modern C# collection expressions.
98-250: Consistent application of required keyword in Game and related classes.The systematic addition of the
requiredmodifier to properties in theFirstInitial,Game, and related classes ensures type safety and consistency throughout the object model.
269-507: Type safety improvements across supporting classes.Adding the
requiredmodifier to properties in classes likeGameOutcome,HomeTeam,LastName,OddsPartner, etc., ensures consistent type safety throughout the schedule object model.
517-685: Enhanced type safety for LeagueSchedule and related classes.The addition of the
requiredmodifier to properties in theLeagueSchedule,SpecialEvent,TvBroadcast,Venue,WinningGoalie, andWinningGoalScorerclasses completes the comprehensive type safety improvements throughout the schedule domain model. The change fromnew()to= []for collection properties also adopts modern C# syntax.Nhl.Api.Domain/Models/Game/GameCenterLanding.cs (1)
25-25: Proper use ofrequiredmodifier enhances type safety across model properties.All reference-type properties across the model classes have been consistently updated with the
requiredmodifier, ensuring these properties must be initialized during object construction. This change strengthens type safety by enforcing non-null values for critical properties, which helps prevent NullReferenceExceptions at runtime.Also applies to: 32-32, 59-59, 66-66, 73-73, 94-94, 113-113, 139-139, 145-145, 151-151, 157-157, 169-169, 175-175, 225-225, 232-232, 246-246, 253-253, 260-260, 267-267, 274-274, 316-316, 323-323, 330-330, 337-337, 343-343, 356-356, 376-376, 383-383, 390-390, 411-411, 424-424, 430-430, 443-443, 462-462, 476-476, 483-483, 497-497, 504-504, 511-511, 518-518, 532-532, 567-567, 573-573, 587-587, 594-594, 601-601, 607-607, 614-614, 621-621, 627-627, 633-633, 674-674, 680-680, 699-699, 705-705, 725-725, 732-732, 766-766, 780-780, 787-787, 794-794, 801-801, 807-807, 813-813, 819-819, 832-832, 838-838, 857-857, 883-883, 889-889, 895-895, 902-902, 908-908, 914-914, 920-920, 926-926, 932-932, 995-995, 1002-1002, 1009-1009, 1147-1147
Nhl.Api.Domain/Models/Player/LastName.cs (1)
14-14: Good type safety improvementAdding the
requiredmodifier to theDefaultproperty ensures it will always be initialized at creation time, which is a good practice for non-nullable reference types. This aligns well with modern C# coding patterns and improves the robustness of the API.Since this could be a breaking change for consumers who previously relied on this property being nullable, the version increment to 3.7.0 (seen in other files) appropriately signals this change.
Nhl.Api.Tests/Nhl.Api.Tests.csproj (2)
4-4: Version increment aligns with semantic versioningThe version increment from 3.6.1 to 3.7.0 appropriately follows semantic versioning principles, indicating the addition of new features or improvements (like the required modifiers) in a backward-compatible way.
5-5:✅ Verification successful
Verify .NET 9.0 readiness for production use
Updating to target .NET 9.0 may be premature as it might not yet be released for general availability or might still be in preview. This could limit the adoption of your library since many users may still be on .NET 8.0.
Consider either:
- Verifying that .NET 9.0 is officially released and stable for production use
- Multi-targeting both .NET 8.0 and 9.0 to maintain broader compatibility
- Waiting until .NET 9.0 reaches general availability before making this change
🌐 Web query:
Is .NET 9.0 released for production use?💡 Result:
Yes, .NET 9.0 has been released for production use. Microsoft officially launched it on November 12, 2024[1][3][4][6][10], and it is supported in production environments under a go-live license[2][13]. While it is a Standard Term Support (STS) release with 18 months of support (ending May 12, 2026)[4][6], it is fully production-ready and offers the same quality and stability as Long-Term Support (LTS) versions like .NET 8[7][13].
Key details:
- Release Date: November 12, 2024[3][4][6].
- Production Readiness: Supported by Microsoft for production workloads, with early adopters running it in production since the Release Candidate (RC) phase[2][7][13].
- Support Policy: STS releases receive free support and patches for 18 months[4][6].
- Key Features: Enhanced Native AOT compilation, cloud-native optimizations, improved ASP.NET Core, and AI integration[1][8][11].
Developers can download the SDK via the .NET website[9] or update through Visual Studio 2022 (v17.12+)[6][9]. While LTS versions like .NET 8 remain preferable for long-term projects, .NET 9 is ideal for teams prioritizing cutting-edge features[6][7].
Citations:
- 1: https://technobrains.io/dot-net-9-features-and-latest-enhancements/
- 2: .NET 9 RC 2 dotnet/core#9534
- 3: https://leobit.com/blog/whats-new-in-net-9-a-business-centric-perspective-on-major-improvements-affecting-net-product-development/
- 4: https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core
- 5: https://dev.to/leandroveiga/boosting-net-9-application-performance-proven-tips-and-techniques-n0m
- 6: https://www.roundthecode.com/dotnet-blog/dotnet-9-released-how-do-you-update
- 7: https://www.youtube.com/watch?v=P0vruEXwkAA
- 8: https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-9/overview
- 9: https://dotnet.microsoft.com/en-us/download/dotnet/9.0
- 10: https://github.com/dotnet/core/blob/main/release-notes/9.0/9.0.0/README.md
- 11: https://www.brainvire.com/blog/dotnet-9-new-features-updates/
- 12: https://versionsof.net
- 13: https://www.devlead.se/posts/2024/2024-09-22-preparing-for-dotnet-9
- 14: https://github.com/dotnet/core/blob/main/release-notes/9.0/preview/preview1/README.md
- 15: https://www.bytehide.com/blog/everything-new-in-net-9-the-ultimate-developers-guide
- 16: https://visualstudiomagazine.com/Articles/2024/04/16/net-9-preview-3.aspx
- 17: https://www.mobilize.net/blog/net-8-vs-net-9-whats-the-difference-and-should-you-migrate
- 18: https://en.wikipedia.org/wiki/.NET
- 19: https://www.roundthecode.com/dotnet-tutorials/whats-new-dotnet-9-key-features-you-need-to-know
- 20: https://modlogix.com/blog/exploring-net-9-the-future-of-cross-platform-software-development/
.NET 9.0 is Production-Ready – Update Accepted
The update in
Nhl.Api.Tests/Nhl.Api.Tests.csprojto target.NET 9.0is appropriate, as Microsoft officially released .NET 9.0 for production use on November 12, 2024. Given that .NET 9.0 is now production-ready, no issues exist with this change. However, if supporting users who prefer the longer-term support of .NET 8.0 is a priority, consider multi-targeting both frameworks.Nhl.Api.Common/Extensions/EnumExtensions.cs (1)
18-18: Improved type safety with nullable return typeChanging the return type from
stringtostring?accurately reflects that this method can return null when no EnumMemberAttribute is found. This makes the API contract more explicit and helps consumers properly handle potential null returns.The implementation already had a null fallback with
?? null, so this change aligns the signature with the actual behavior, improving type safety without changing functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Nhl.Api.Tests/PlayerTests.cs (1)
433-441:⚠️ Potential issueUpdate expected exception type for incorrect player type test.
The test expects an HttpRequestException, but according to the previous review comments, the implementation throws an ArgumentException for incorrect player types. This test will likely fail and should be updated.
- _ = Assert.ThrowsExceptionAsync<HttpRequestException>(async () => await nhlApi.GetGoalieInformationAsync(PlayerEnum.SidneyCrosby8471675)); + _ = Assert.ThrowsExceptionAsync<ArgumentException>(async () => await nhlApi.GetGoalieInformationAsync(PlayerEnum.SidneyCrosby8471675));
🧹 Nitpick comments (1)
Nhl.Api.Domain/Services/NhlGameService.cs (1)
30-30: Good change to use concrete type and target-typed new expression.The change from using the interface type
INhlApiHttpClientto the concrete typeNhlScoresHtmlReportsApiHttpClientmakes the code more explicit about which implementation is being used. This improves code clarity and potentially enables better compiler optimizations. The target-typednew()expression is also a welcome modernization of the syntax.However, consider whether this might impact testability—using an interface would allow for easier mocking during unit tests. If testability is important for this component, you might want to consider maintaining the interface as the field type while still using the concrete implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Nhl.Api.Domain/Services/NhlGameService.cs(3 hunks)Nhl.Api.Tests/PlayerTests.cs(12 hunks)
🧰 Additional context used
🪛 GitHub Actions: Develop Build
Nhl.Api.Tests/PlayerTests.cs
[error] 150-150: Test method Nhl.Api.Tests.PlayerTests.SearchAllActivePlayersAsync_Returns_Valid_Information threw exception: System.ArgumentException: Value does not fall within the expected range. (Parameter 'query')
🔇 Additional comments (7)
Nhl.Api.Domain/Services/NhlGameService.cs (2)
170-170: Correctly removed 'this' keyword for static method call.This change properly aligns with making the
CalculateMultipliermethod static, as seen in line 195. The removal of thethiskeyword is appropriate since static methods are called on the class rather than on an instance.
195-195: Good refactoring to make method static.Making
CalculateMultiplierstatic is an excellent improvement since the method doesn't use any instance state - it only operates on its parameters. This change:
- Makes the code's intent clearer
- Slightly improves performance (no
thisreference passed)- Prevents accidental use of instance variables
This is a good application of the principle that methods that don't depend on instance state should be static.
Nhl.Api.Tests/PlayerTests.cs (5)
162-162: LGTM: Explicit use of discard operator.Good use of the discard operator (
_) to explicitly indicate that you're ignoring the result ofAssert.ThrowsExceptionAsync. This follows best practices for making intent clear in the code.Also applies to: 172-172, 212-212, 225-225, 364-364, 378-378, 394-394, 409-409, 824-824, 946-946, 957-957
292-302: LGTM: Good test for invalid season years.This test properly verifies that the API throws an ArgumentException when invalid season year formats are provided. This enhances the robustness of the test suite.
412-431: LGTM: Good validation of goalie statistics.This test thoroughly validates the correct statistics are returned for a goalie player, checking various properties of the response. This is a valuable addition to the test suite.
918-926: LGTM: Good resource disposal testing.Adding tests for proper disposal of resources is an excellent practice. These tests ensure that both the synchronous
Dispose()and asynchronousDisposeAsync()methods properly clean up resources.Also applies to: 929-937
940-958: LGTM: Thorough validation of input parameters.These tests properly verify that the API validates input parameters and throws appropriate exceptions for invalid inputs. This is important for maintaining API robustness.
- Filter player search result by name for precise assertion. - Correct typo from "Joesph Woll" to "Joseph Woll". - Update `BirthProvinceState` for "Joseph Woll" to "MO".
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Modified the Dispose and DisposeAsync methods in the NhlApi class to include calls to _nhlPlayerApi.Dispose() before GC.SuppressFinalize(this). This ensures that _nhlPlayerApi resources are properly disposed of, preventing potential resource leaks both synchronously and asynchronously.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (1)
72-73: Consider verifying the playoff start date.
This condition checks only the regular season end and the playoff end, potentially leaving a gap if the playoffs haven't begun yet. If intended, no further action required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Nhl.Api.Tests/PlayerTests.cs(14 hunks)Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: build
🔇 Additional comments (29)
Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (11)
50-50: Good use of CultureInfo for date parsing.
No issues seen, as this prevents locale-specific parsing bugs.
95-95: Consistent date parsing with CultureInfo.
This ensures uniform date interpretations across systems. No concerns here.
111-111: Improved exception specificity.
Replacing a generic error with anInvalidTeamAbbreviationExceptionis a clean way to provide clarity and consistent error handling.
135-135: Consistent custom exception usage.
Again, good move to employInvalidTeamAbbreviationExceptionhere for invalid abbreviations.
561-561: Appending with InvariantCulture is appropriate.
No issues found. This aligns with best practices for culture-agnostic text formatting.
568-568: Confirm consistent usage of InvariantCulture.
Good approach to avoid locale issues in appended strings.
572-572: Safe string append with InvariantCulture.
No concerns; this ensures a predictable format across locales.
599-599: CultureInfo invariant string building.
Consistent with the rest of the method. Looks good.
603-603: Retaining consistent format behavior.
All string-joining code now uses InvariantCulture. No issues.
615-616: Streamlined league active check.
Combining these asynchronous checks with boolean OR logic is concise and maintainable.
617-631: New method to retrieve playoff series by season year.
Parameter validation for the eight-digit format is effective. The previously noted hard-coded year has been replaced with the parameter—great fix. No further issues found.Nhl.Api.Tests/PlayerTests.cs (18)
2-2: Added HttpClient reference.
This import is presumably used for exception checking in tests. No concerns.
93-114: Refined test logic for specific player name searches.
UsingFirst(r => r.Name == query)ensures the test matches the expected player exactly. Corrections for “Joseph Woll” and province “MO” look accurate.
163-163: Valid test for invalid season year.
AssertingArgumentExceptionensures robust input validation.
172-172: Consistent exceptions for invalid arguments.
Same pattern as above. Confirms input validation flows are consistent.
212-212: Assert on null season year guards.
Helps check boundary conditions effectively.
225-225: Empty season year check.
Maintains consistency in invalid input handling. No issues.
292-302: Testing invalid season year thoroughly.
Covers both whitespace and incorrect length. Ensures better reliability.
364-364: ArgumentException on malformed year.
Continues consistent approach for invalid data. Good coverage.
378-378: Empty year check for goalie logs.
Matches the same approach used for player logs. No concerns.
383-395: Expanded invalid format test for goalie ID logs.
DataRow usage enhances coverage of multiple inputs easily.
398-410: Empty year for goalie logs.
Again, consistent with existing patterns and ensures thorough validation.
413-431: New test verifying goalie stats retrieval.
Solid coverage of typical properties. Ensures correct data mapping.
434-441: Inconsistent exception expectation for non-goalie.
Test expectsHttpRequestException. However, code or design might suggestArgumentExceptioncould be more appropriate for invalid input. Double-check whether the server call triggers an HTTP error or an input validation error.Would you like a script to confirm if the underlying service truly returns an HTTP error for a forward’s ID or if it fails earlier with an argument exception?
626-626: Ensures invalid ID triggersArgumentException.
Good negative test coverage confirming the guard clause behavior.
917-926: Async disposal test added.
Verifies thatIAsyncDisposablelogic cleans resources as intended. No concerns.
929-937: Sync disposal test added.
Ensures standardDispose()also frees resources. Aligned with best practices.
939-947: Draft ranking: invalid season year test.
Checking whitespace ensures coverage for minimal input errors. Implementation is correct.
951-957: Draft ranking: invalid starting position test.
Guard clause is validated properly with anArgumentException. Good negative testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Nhl.Api/Src/Api/NhlApi.cs (1)
749-753: Recommend improving DisposeAsync implementationWhile the explicit disposal of
_nhlPlayerApiis good, there are two improvements that could be made to the DisposeAsync method:
- The method should await the asynchronous disposal of
_nhlPlayerApiif it implementsIAsyncDisposable- Using
Task.RunforGC.SuppressFinalizeis unnecessary and creates extra overheadpublic async ValueTask DisposeAsync() { - _nhlPlayerApi.Dispose(); - await Task.Run(() => GC.SuppressFinalize(this)); + if (_nhlPlayerApi is IAsyncDisposable asyncDisposable) + { + await asyncDisposable.DisposeAsync(); + } + else + { + _nhlPlayerApi.Dispose(); + } + GC.SuppressFinalize(this); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Nhl.Api/Src/Api/NhlApi.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: build
🔇 Additional comments (2)
Nhl.Api/Src/Api/NhlApi.cs (2)
739-743: Improved resource cleanup in Dispose methodThe changes properly address the resource disposal issue that was identified in the previous review. Now
_nhlPlayerApiis explicitly disposed before callingGC.SuppressFinalize(this).
755-762: New method implementation looks goodThe new
GetPlayoffSeriesBySeasonYearAsyncmethod follows the established pattern of other methods in this class. It has proper XML documentation and parameter handling, delegating to the appropriate API instance.Consider adding parameter validation for the
seasonYearparameter to ensure it's in the correct format before passing it to the league API. This would maintain consistency with other methods in the codebase.
- Add `using System;` to `CountryCodeHelper.cs` - Throw `ArgumentException` for invalid country codes - Initialize `Series` property with an empty array in `PlayoffSeriesSchedule.cs`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Nhl.Api.Domain/Models/Schedule/PlayoffSeriesSchedule.cs (4)
6-7: Remove unnecessary empty lines.There are two consecutive empty lines here that serve no purpose. Consider removing one of them to maintain consistent spacing throughout the codebase.
17-17: Consider making Id non-nullable if it's a critical identifier.If the team Id is a fundamental identifier that should always be present for a valid team, consider making this property non-nullable. This would improve type safety and reduce the need for null checks in consumers of this API.
- public int? Id { get; set; } + public int Id { get; set; }
171-171: Fix documentation formatting inconsistency.The format of the example is inconsistent with other property documentation. There should be a space between the description and the example.
- /// Gets or sets the number of wins required to win the series.<br/>Example: 4 + /// Gets or sets the number of wins required to win the series.<br/> Example: 4
52-53: Consider adding XML documentation for inherited properties.The
BottomSeedandTopSeedclasses inherit properties from theSeedbase class but don't include<inheritdoc/>tags. While the IDE can provide inheritance information, explicit documentation inheritance improves code generation and documentation tools.public class BottomSeed : Seed { + /// <inheritdoc/> } public class TopSeed : Seed { + /// <inheritdoc/> }Also applies to: 59-60
Nhl.Api.Common/Helpers/CountryCodeHelper.cs (1)
14-1253: Consider refactoring to use a lookup Dictionary for better performanceThe current implementation uses a long series of if-statements for country code lookup, which has O(n) complexity where n is the number of countries. Using a Dictionary would provide O(1) lookup time.
Here's a more efficient implementation using a Dictionary:
public static string ConvertThreeDigitCountryCodeToFullCountryName(string countryCode) { - if (countryCode == "AFG") - { - return "Afghanistan"; - } - - // [hundreds of other if statements...] - - throw new ArgumentException($"Country code {countryCode} not found"); + if (string.IsNullOrEmpty(countryCode)) + { + throw new ArgumentException("Country code cannot be null or empty"); + } + + var countryCodes = new Dictionary<string, string> + { + { "AFG", "Afghanistan" }, + { "ALA", "Aland Islands" }, + // [all other country codes] + { "ZWE", "Zimbabwe" } + }; + + if (countryCodes.TryGetValue(countryCode, out var fullName)) + { + return fullName; + } + + throw new ArgumentException($"Country code {countryCode} not found"); }This approach offers:
- Better performance with O(1) lookup
- Cleaner, more maintainable code
- Easier to add new country codes or make changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Nhl.Api.Common/Helpers/CountryCodeHelper.cs(2 hunks)Nhl.Api.Domain/Models/Schedule/PlayoffSeriesSchedule.cs(1 hunks)Nhl.Api.Tests/AssemblyInfo.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: build
🔇 Additional comments (6)
Nhl.Api.Tests/AssemblyInfo.cs (2)
4-4: Good enhancement by adding thesealedmodifierAdding the
sealedmodifier to theAssemblyInfoclass is a good practice as it clearly communicates that this class is not intended to be inherited from. This can also potentially allow for compiler optimizations.
1-1: LGTM: Test parallelization configuration is properly maintainedThe
Parallelizeattribute withWorkers = 4, Scope = ExecutionScope.MethodLevelis appropriately configured to run tests in parallel with 4 worker threads at the method level, which helps optimize test execution performance.Nhl.Api.Domain/Models/Schedule/PlayoffSeriesSchedule.cs (2)
10-175: Well-structured domain model with strong typing.The overall structure of these domain models is excellent. Good practices observed:
- Proper use of class hierarchy with an abstract base class
- Consistent and detailed XML documentation with examples
- Appropriate use of the
requiredmodifier for non-nullable properties- Default initialization of collections with the new C# collection syntax
- Proper JSON property mapping
These models will provide strong type safety and clear documentation for consumers of your API.
86-86: Good use of collection initialization syntax.The use of the new C# collection initialization syntax
= []is excellent. It ensures collections are never null, reducing the risk of NullReferenceExceptions and simplifying consumer code by eliminating the need for null checks.Also applies to: 120-120
Nhl.Api.Common/Helpers/CountryCodeHelper.cs (2)
1-1252: Code formatting improvements enhance readabilityThe addition of braces around each conditional statement and blank lines between country code blocks significantly improves code readability and maintainability.
1252-1252: Update unit tests for the new exception behaviorThe previous review comments mentioned a need to update unit tests for the change from returning
nullto an empty string. Now that the method throws an exception, the unit tests need to be updated again.Make sure you add unit tests that:
- Verify the method throws
ArgumentExceptionfor unrecognized country codes- Test that the exception message contains the unrecognized country code
#!/bin/bash # Search for existing unit tests for this method rg "ConvertThreeDigitCountryCodeToFullCountryName" -g "*.Tests.cs" -A 5 -B 5
Updated AssemblyInfo.cs to increase the number of workers for parallel execution from 4 to 16 by modifying the Workers parameter in the Parallelize attribute. The Scope parameter remains set to ExecutionScope.MethodLevel.
No description provided.