v3.4.0 #52
Merged
v3.4.0 #52
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThe update to the NHL API involves transitioning to .NET framework 8.0, improving player and goalie statistics retrieval, refining method parameters for clarity, enhancing project structure and testing, adjusting HTTP client and service instantiation, and streamlining .NET setup in GitHub workflows by exclusively targeting version 8. These changes signify a comprehensive effort to modernize the API's infrastructure and functionalities. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (14)
- Nhl.Api.Common/Nhl.Api.Common.csproj (1 hunks)
- Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs (1 hunks)
- Nhl.Api.Domain/Models/Player/ExpressionPlayerFilterBuilder.cs (1 hunks)
- Nhl.Api.Domain/Models/Player/ShootsCatches.cs (1 hunks)
- Nhl.Api.Domain/Models/Season/SeasonYears.cs (1 hunks)
- Nhl.Api.Domain/Models/Statistics/GoalieStatisticsFilterResult.cs (1 hunks)
- Nhl.Api.Domain/Models/Statistics/PlayerStatisticsFilterResult.cs (1 hunks)
- Nhl.Api.Domain/Nhl.Api.Domain.csproj (1 hunks)
- Nhl.Api.Tests/Nhl.Api.Tests.csproj (1 hunks)
- Nhl.Api.Tests/StatisticsTests.cs (2 hunks)
- Nhl.Api/Nhl.Api.csproj (1 hunks)
- Nhl.Api/Src/Api/NhlApi.cs (2 hunks)
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs (1 hunks)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (4 hunks)
Files skipped from review due to trivial changes (3)
- Nhl.Api.Common/Nhl.Api.Common.csproj
- Nhl.Api.Domain/Models/Player/ShootsCatches.cs
- Nhl.Api.Domain/Nhl.Api.Domain.csproj
Additional comments: 26
Nhl.Api.Tests/Nhl.Api.Tests.csproj (2)
- 4-4: The update of the project version to 3.4.0 aligns with the PR's objectives and ensures consistency across the project's components.
- 4-4: Updating the target framework to .NET 8.0 is a positive step towards modernizing the project. Ensure compatibility with all dependencies and project references.
Nhl.Api/Nhl.Api.csproj (1)
- 4-4: The update of the project version to 3.4.0 and the targeting of multiple .NET frameworks (6.0, 7.0, and 8.0) are significant improvements that align with the PR's objectives. Ensure that the project's code and dependencies are compatible across these frameworks.
Nhl.Api.Domain/Models/Statistics/GoalieStatisticsFilterResult.cs (1)
- 1-190: The introduction of
GoalieStatisticsFilterResultandGoalieStatisticsResultclasses, along with their detailed documentation, significantly enhances the API's capabilities for querying and filtering NHL goalie statistics. Ensure that the data types and JSON annotations align with the expected data format and usage.Nhl.Api.Domain/Models/Statistics/PlayerStatisticsFilterResult.cs (1)
- 1-211: The introduction of
PlayerStatisticsFilterResultandPlayerStatisticsResultclasses, along with their detailed documentation, significantly enhances the API's capabilities for querying and filtering NHL player statistics. Ensure that the data types and JSON annotations align with the expected data format and usage.Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs (8)
- 29-29: The class
GoalieFilterExpressionBuilderis well-named, clearly indicating its purpose as a builder for goalie filter expressions. This aligns with the builder pattern, facilitating the construction of complex objects step by step.- 31-31: The initialization of
_filterExpressionwith the prefix "factCayenneExp=" is specific and implies integration with a particular query language or API. Ensure that this prefix aligns with the expected format for the underlying data source or API to which these expressions will be submitted.- 70-74: The
Andmethod correctly appends the logical operator "and" to the filter expression. This method is straightforward and follows the builder pattern, allowing for chaining additional filter conditions.- 80-84: The
Ormethod functions similarly to theAndmethod, appending the logical operator "or" to the filter expression. It adheres to the builder pattern, enabling the combination of filter conditions with an OR logic.- 90-94: The
StartGroupandEndGroupmethods (lines 90-94 and 100-104) provide functionality for grouping conditions within the filter expression using parentheses. This is a useful feature for constructing more complex logical conditions. Ensure that these methods are used correctly to maintain the proper balance of opening and closing parentheses in the filter expression.- 185-188: The
Buildmethod correctly creates anExpressionGoalieFilterinstance with the current state of the filter expression. This method encapsulates the construction process, providing a clean interface for obtaining the final product of the builder.- 193-193: The
Emptystatic property provides a convenient way to obtain an emptyExpressionGoalieFilter. This is a useful feature for cases where no filtering is required. Ensure that this property is used appropriately in the context of the application.- 238-400: The
GoalieStatisticsFilterenum defines a comprehensive set of filterable properties for goalie statistics. Each enum member is annotated with bothDescriptionandEnumMemberattributes, which is good practice for ensuring compatibility with different serialization formats and providing descriptive names for the enum values.Nhl.Api.Domain/Models/Season/SeasonYears.cs (1)
- 453-476: The addition of constants for NHL seasons from 2029-2030 to 2034-2035 correctly follows the established naming and formatting conventions within the
SeasonYearclass. This update aligns with the PR's objective of extending support for future NHL seasons, ensuring the API's utility in the coming years.Nhl.Api.Tests/StatisticsTests.cs (9)
- 5-7: The addition of imports for
Nhl.Api.Models.PlayerandNhl.Api.Models.Teamis appropriate given the context of the added test methods that likely utilize these models. Ensure that these imports are used within the test methods to avoid unnecessary imports.- 431-463: The test method
GetPlayerStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_Querydemonstrates a good practice of arranging, acting, and asserting within a test. However, there are a couple of points to consider:
- The comment on line 453 should be "Assert" instead of "Act" as it's part of the assertion phase of the test.
- Ensure that the
PlayerFilterExpressionBuilderand the methodGetPlayerStatisticsBySeasonAndFilterExpressionAsynchandle edge cases and exceptions gracefully. For example, what happens if the season year provided does not exist or if no players meet the filter criteria?- // Act + // AssertConsider adding tests to cover edge cases and exception handling scenarios for the
GetPlayerStatisticsBySeasonAndFilterExpressionAsyncmethod.
- 467-494: This test method,
GetPlayerStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_Query_For_2000_Season, is well-structured and follows the arrange-act-assert pattern effectively. However, ensure that historical data (e.g., the 1999-2000 season) is accurately represented and available in the API's database to avoid potential data availability issues.Verify the availability and accuracy of historical NHL data within the API's database to ensure the test's validity.
- 498-512: The test method
GetPlayerStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Empty_Expressionis an excellent example of testing the API's behavior with minimal filtering criteria. It's important to ensure that the API and underlying data handling mechanisms are optimized for such broad queries to prevent performance issues.Consider verifying the performance and scalability of handling broad queries, especially when minimal filtering criteria are applied, to ensure the API remains responsive under load.
- 517-558: In the test method
GetPlayerStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_Query_For_Complex_Query_1, the complex filter expression built for the test is a good practice for testing the robustness of the filtering functionality. However, it's crucial to:
- Ensure that the API's filtering logic accurately interprets and applies complex filter expressions.
- Review the maintainability of such complex expressions and consider simplifying them if possible to improve readability and ease of understanding.
Verify the accuracy of the API's filtering logic in interpreting and applying complex filter expressions. Additionally, assess the maintainability and readability of complex filter expressions within test methods.
- 562-613: Similar to the previous comment, the test method
GetPlayerStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_Query_For_Complex_Query_2effectively tests complex filtering logic. It's important to:
- Ensure that grouping logic within filter expressions (e.g., using
StartGroup,EndGroup) is correctly implemented and interpreted by the API.- Consider the readability and maintainability of tests with complex filter expressions, especially when using grouping logic.
Verify the correct implementation and interpretation of grouping logic within filter expressions by the API. Assess the readability and maintainability of tests involving complex filter expressions and grouping logic.
- 618-628: The test method
GetGoalieStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_With_No_Queryis crucial for testing the API's default behavior when no specific filters are applied. It's important to:
- Ensure that the API and underlying data handling mechanisms can efficiently handle queries without specific filters, potentially returning large datasets.
- Consider adding more detailed assertions to verify the correctness of the returned data, beyond just checking the total number of results.
Verify the efficiency and scalability of the API when handling queries without specific filters. Consider enhancing the assertions in this test method to verify the correctness of the returned data in more detail.
- 632-657: The test method
GetGoalieStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_Query_For_Simple_Queryeffectively tests the API's filtering functionality with a simple query. It's important to:
- Ensure that simple filter expressions are accurately interpreted and applied by the API.
- Review the test for potential edge cases or scenarios where the filtering criteria might not yield any results, and ensure that the API handles these gracefully.
Verify the accuracy of the API's filtering logic for simple filter expressions. Additionally, assess how the API handles scenarios where filtering criteria do not yield any results.
- 663-701: The test method
GetGoalieStatisticsBySeasonAndFilterAsync_Returns_Valid_Results_Based_On_Filter_Query_For_Complex_Queryis another example of testing complex filtering logic. It's important to:
- Ensure that the API's filtering logic accurately interprets and applies complex filter expressions, including grouping logic.
- Review the maintainability and readability of such complex expressions within test methods, and consider simplifying them if possible.
Verify the accuracy of the API's filtering logic in interpreting and applying complex filter expressions, including grouping logic. Assess the maintainability and readability of complex filter expressions within test methods.
Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (1)
- 26-26: The addition of a new
INhlApiHttpClientinstance_nhlEApiWebHttpClientis noted. Ensure that this new client is properly configured and that its usage is consistent with the existing_nhlApiWebHttpClientin terms of error handling, timeouts, and other HTTP client configurations.Nhl.Api/Src/Api/NhlApi.cs (2)
- 798-811: The method
GetPlayerStatisticsBySeasonAndFilterExpressionAsyncintroduces a powerful way to query player statistics with filters. However, there are a few points to consider:
- The default value of
limit = -1might be confusing. It's common to set a reasonable default limit to prevent excessive data retrieval that could impact performance. Consider setting a positive default value or documenting the behavior of-1.- Ensure that the
ExpressionPlayerFilterandPlayerStatisticsFilterare well-documented, especially regarding how users can construct these filters.- It's good practice to validate input parameters, such as
seasonYear, to ensure they match expected formats or values.- Consider adding examples in the method documentation to demonstrate how to use this method effectively.
- 813-826: The method
GetGoalieStatisticsBySeasonAndFilterExpressionAsyncmirrors the player statistics method for goalies, which is a great addition for consistency. Similar considerations apply:
- Reevaluate the default
limit = -1for reasons mentioned previously.- Ensure
ExpressionGoalieFilterandGoalieStatisticsFilterare well-documented for ease of use.- Input validation for
seasonYearshould be considered to prevent potential errors.- Including usage examples in the documentation would greatly benefit users unfamiliar with constructing filters.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (6)
- Nhl.Api.Tests/PlayerTests.cs (2 hunks)
- Nhl.Api/Src/Api/NhlApi.cs (4 hunks)
- Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (3 hunks)
- Nhl.Api/Src/PlayerApi/INhlPlayerApi.cs (2 hunks)
- Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (7 hunks)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (8 hunks)
Files skipped from review as they are similar to previous changes (2)
- Nhl.Api/Src/Api/NhlApi.cs
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs
Additional comments: 6
Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (2)
- 8-8: Adding the
Nhl.Api.Servicesnamespace is appropriate for accessing services within the Nhl.Api project. This change supports the introduction of new service dependencies in theNhlPlayerApiclass.- 23-25: Renaming
_nhlCmsHttpClientto_nhlStaticAssetsApiHttpClientand introducing_nhlTeamServiceimprove the clarity and specificity of these variables. The new names better reflect the purpose and usage of these dependencies within theNhlPlayerApiclass.Nhl.Api.Tests/PlayerTests.cs (4)
- 646-649: The change from
PlayerHeadshotImageSizeenum tostring seasonYearin theDataRowattributes forTestDownloadPlayerHeadshotImageAsyncmethod aligns with the PR objectives to enhance the API's querying capabilities by allowing filtering based on season years. This change is consistent and logical, ensuring the method tests functionality with various season years.- 655-655: The update to use
PlayerEnum.ConnorMcDavid8478402andseasonYearas parameters inGetPlayerHeadshotImageAsynccall is correct and aligns with the updated method signature. This ensures the test is now more flexible, allowing for testing headshot image downloads across different seasons.- 664-667: Similar to the previous comment, changing the parameter type in
GetPlayerHeadshotImageAsync_TestDownload_PlayerHeadshot_ImageWithIdmethod fromPlayerHeadshotImageSizeenum tostring seasonYearis consistent with the PR's goal to enhance filtering capabilities. This change allows for testing the functionality with various season years, which is a positive improvement.- 673-673: The update to use a specific player ID (
8477932) andseasonYearas parameters in theGetPlayerHeadshotImageAsynccall withinGetPlayerHeadshotImageAsync_TestDownload_PlayerHeadshot_ImageWithIdmethod is correct. It ensures the test checks the functionality for downloading player headshot images across different seasons for a specific player ID.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 88.17% 88.70% +0.53%
==========================================
Files 72 75 +3
Lines 3112 3276 +164
Branches 419 438 +19
==========================================
+ Hits 2744 2906 +162
+ Misses 305 303 -2
- Partials 63 67 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs (1 hunks)
- Nhl.Api.Domain/Models/Player/ExpressionPlayerFilterBuilder.cs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs
- Nhl.Api.Domain/Models/Player/ExpressionPlayerFilterBuilder.cs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- Nhl.Api.Tests/StatisticsTests.cs (6 hunks)
- Nhl.Api.Tests/StringTests.cs (1 hunks)
- Nhl.Api/Src/Api/NhlApi.cs (4 hunks)
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs (1 hunks)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (8 hunks)
Files skipped from review as they are similar to previous changes (4)
- Nhl.Api.Tests/StatisticsTests.cs
- Nhl.Api/Src/Api/NhlApi.cs
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs
Additional comments: 4
Nhl.Api.Tests/StringTests.cs (4)
- 40-112: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-25]
The test method
ReplaceNonAsciiWithAscii_Returns_Correct_Ascii_Stringis well-structured and covers a wide range of input scenarios. It effectively validates the functionality of theReplaceNonAsciiWithAsciimethod, ensuring that it correctly converts non-ASCII characters to their closest ASCII equivalents.
- 40-112: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [26-42]
The test method
ReplaceNonAsciiWithAscii_Returns_Correct_Ascii_String_Extensioneffectively validates the extension method version ofReplaceNonAsciiWithAscii. It ensures that the extension method behaves consistently with the static method, covering a variety of input scenarios.
- 43-83: The test method
ReplaceNonAsciiWithAscii_Testsignificantly expands the range of test cases to include a broader array of non-ASCII characters. This thorough testing approach is crucial for ensuring the method's reliability across various languages and character sets.- 85-111: The test methods
ReplaceNonAsciiWithAscii_WithEmptyString_TestandReplaceNonAsciiWithAscii_WithOnlyAsciiCharacters_Testeffectively cover important edge cases: handling an empty string and a string with only ASCII characters. Including these edge cases in the test suite enhances the method's reliability and robustness.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- Nhl.Api.Tests/PlayerTests.cs (3 hunks)
- Nhl.Api/README.md (16 hunks)
- Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (7 hunks)
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs (1 hunks)
Files not summarized due to errors (1)
- Nhl.Api/README.md: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (2)
- Nhl.Api.Tests/PlayerTests.cs
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs
Additional comments: 5
Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (4)
- 8-8: The addition of the
Nhl.Api.Servicesnamespace supports the introduction of new services, aligning with the PR's objectives to enhance API capabilities.- 22-22: Renaming
_nhlCmsHttpClientto_nhlStaticAssetsApiHttpClientimproves clarity and better reflects the variable's role.- 24-24: The addition of
_nhlTeamServicesupports new team-related functionalities, aligning with the PR's objectives.- 77-80: Updates to parameter validation across methods, focusing on the
seasonYearformat, enhance the API's robustness by ensuring data integrity and proper user input handling. This is a positive change that aligns with best practices for input validation.Also applies to: 103-106, 135-135, 158-158, 181-181, 204-204
Nhl.Api/README.md (1)
- 2800-2821: The addition of the
GetPlayerStatisticsBySeasonAndFilterExpressionAsyncmethod in theNhlStatisticsApiclass is a valuable enhancement. It extends the API's capabilities by allowing users to fetch detailed player statistics for a specific season with customizable filtering and sorting. This method supports advanced data analysis and application features that require granular player performance data. The parameters for filtering and sorting are well-thought-out, providing flexibility in data retrieval. This update aligns with the project's goal of offering comprehensive NHL data access.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (13)
- Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs (1 hunks)
- Nhl.Api.Tests/PlayerTests.cs (3 hunks)
- Nhl.Api.Tests/ProjectStructureTests.cs (1 hunks)
- Nhl.Api/Src/Api/NhlApi.cs (6 hunks)
- Nhl.Api/Src/GameApi/INhlGameApi.cs (1 hunks)
- Nhl.Api/Src/GameApi/NhlGameApi.cs (2 hunks)
- Nhl.Api/Src/GlobalUsings.cs (1 hunks)
- Nhl.Api/Src/LeagueApi/INhlLeagueApi.cs (2 hunks)
- Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (6 hunks)
- Nhl.Api/Src/PlayerApi/INhlPlayerApi.cs (3 hunks)
- Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (7 hunks)
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs (2 hunks)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (9 hunks)
Files skipped from review due to trivial changes (1)
- Nhl.Api/Src/GameApi/INhlGameApi.cs
Files skipped from review as they are similar to previous changes (8)
- Nhl.Api.Domain/Models/Player/ExpressionGoalieFilterBuilder.cs
- Nhl.Api.Tests/PlayerTests.cs
- Nhl.Api.Tests/ProjectStructureTests.cs
- Nhl.Api/Src/Api/NhlApi.cs
- Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs
- Nhl.Api/Src/PlayerApi/INhlPlayerApi.cs
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs
Additional comments: 7
Nhl.Api/Src/GlobalUsings.cs (1)
- 5-12: The addition of global usings for
System.Linq,System.Text,System.Threading,Nhl.Api.Common.Extensions,Nhl.Api.Common.Helpers, andNhl.Api.Common.Httpis a good practice for reducing redundancy across the project. However, ensure that these namespaces are indeed used across multiple files in the project to justify their global inclusion.Nhl.Api/Src/LeagueApi/INhlLeagueApi.cs (2)
- 149-149: The updated return statement for
GetTeamProspectsByTeamAsyncmethod provides clearer documentation on what the method returns, enhancing the API's usability. This is a good practice for API documentation.- 157-157: Similarly, the return statement for the overloaded
GetTeamProspectsByTeamAsyncmethod withTeamEnumparameter is also improved, providing clear expectations for API consumers. This consistency in documentation across overloads is commendable.Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (4)
- 7-7: The addition of the
Nhl.Api.Servicesnamespace is appropriate for the context of the changes, ensuring that the newly introduced services are properly organized and accessible.- 16-21: Renaming
_nhlCmsHttpClientto_nhlStaticAssetsApiHttpClientand adding_nhlTeamServiceimproves the clarity and specificity of the variable names, aligning them more closely with their purposes and functionalities.- 129-135: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [132-155]
The validation logic for
seasonYearin methodsGetPlayerSeasonGameLogsBySeasonAndGameTypeAsyncandGetGoalieSeasonGameLogsBySeasonAndGameTypeAsyncfor both player and goalie statistics retrieval is consistent and ensures that theseasonYearparameter is in the correct format. This is crucial for the accuracy of data retrieval based on the season year.
- 175-181: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [178-201]
The consistency in handling
seasonYearvalidation across different methods, includingGetGoalieSeasonGameLogsBySeasonAndGameTypeAsync, demonstrates attention to detail and ensures that the API behaves predictably when handling season-based queries. This consistency is key to maintaining a reliable and user-friendly API.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- Nhl.Api.Tests/GlobalUsings.cs (1 hunks)
- Nhl.Api.Tests/ProjectStructureTests.cs (1 hunks)
- Nhl.Api/README.md (17 hunks)
Files not summarized due to errors (1)
- Nhl.Api/README.md: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- Nhl.Api.Tests/GlobalUsings.cs
Files skipped from review as they are similar to previous changes (1)
- Nhl.Api.Tests/ProjectStructureTests.cs
Additional comments: 4
Nhl.Api/README.md (4)
- 120-126: The addition of new methods to the
NhlApiclass is documented here, which is good for keeping the README up-to-date with the latest API capabilities. However, it would be beneficial to include a brief description of what each method does for better clarity to the users.Consider adding a one-line description for each method listed under the
NhlApiclass to provide users with a quick understanding of the method's purpose.
- 117-129: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [126-144]
The documentation for the newly added methods in the
NhlApiclass is clear and follows the existing format. However, it's important to ensure consistency in the documentation style across the entire README.The method names and parameters are correctly documented, which will help users understand how to use the new features.
- 141-147: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [144-154]
The addition of goalie-specific statistics retrieval methods is a significant enhancement to the API. The documentation correctly lists these new methods.
The inclusion of these methods will provide users with more detailed statistics, enhancing the API's utility for hockey analytics.
- 154-161: The methods for retrieving player headshot images have been updated to include a
seasonYearparameter, which is a useful enhancement for retrieving season-specific images.This change allows for more precise retrieval of player images, which can be particularly useful for historical data analysis or season-specific applications.
….net ready to be deprecated and added tests
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (20)
- Nhl.Api.Common/Nhl.Api.Common.csproj (1 hunks)
- Nhl.Api.Domain/Enumerations/Player/PlayerEnum.cs (1 hunks)
- Nhl.Api.Domain/Enumerations/Player/PlayerEnumFileGeneratorHelper.cs (1 hunks)
- Nhl.Api.Domain/Nhl.Api.Domain.csproj (1 hunks)
- Nhl.Api.Tests/GlobalUsings.cs (1 hunks)
- Nhl.Api.Tests/Nhl.Api.Tests.csproj (1 hunks)
- Nhl.Api.Tests/PlayerTests.cs (1 hunks)
- Nhl.Api.Tests/ProjectStructureTests.cs (1 hunks)
- Nhl.Api.Tests/StringTests.cs (2 hunks)
- Nhl.Api/Nhl.Api.csproj (1 hunks)
- Nhl.Api/Src/Api/NhlApi.cs (4 hunks)
- Nhl.Api/Src/GameApi/INhlGameApi.cs (1 hunks)
- Nhl.Api/Src/GameApi/NhlGameApi.cs (2 hunks)
- Nhl.Api/Src/GlobalUsings.cs (1 hunks)
- Nhl.Api/Src/LeagueApi/INhlLeagueApi.cs (2 hunks)
- Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs (5 hunks)
- Nhl.Api/Src/PlayerApi/INhlPlayerApi.cs (3 hunks)
- Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (7 hunks)
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs (2 hunks)
- Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (8 hunks)
Files not summarized due to errors (1)
- Nhl.Api.Tests/PlayerTests.cs: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (14)
- Nhl.Api.Common/Nhl.Api.Common.csproj
- Nhl.Api.Domain/Nhl.Api.Domain.csproj
- Nhl.Api.Tests/GlobalUsings.cs
- Nhl.Api.Tests/Nhl.Api.Tests.csproj
- Nhl.Api.Tests/ProjectStructureTests.cs
- Nhl.Api.Tests/StringTests.cs
- Nhl.Api/Nhl.Api.csproj
- Nhl.Api/Src/GameApi/INhlGameApi.cs
- Nhl.Api/Src/GameApi/NhlGameApi.cs
- Nhl.Api/Src/GlobalUsings.cs
- Nhl.Api/Src/LeagueApi/INhlLeagueApi.cs
- Nhl.Api/Src/LeagueApi/NhlLeagueApi.cs
- Nhl.Api/Src/PlayerApi/INhlPlayerApi.cs
- Nhl.Api/Src/StatisticsApi/INhlStatisticsApi.cs
Additional comments: 28
Nhl.Api.Domain/Enumerations/Player/PlayerEnum.cs (1)
- 1-1: The namespace declaration is correctly updated to
Nhl.Api.Models.Enumerations.Player, which aligns with the project's structure for organizing player-related enumerations. This change enhances the maintainability and clarity of the codebase.Nhl.Api.Domain/Enumerations/Player/PlayerEnumFileGeneratorHelper.cs (1)
- 51-51: Adding the namespace declaration
Nhl.Api.Models.Enumerations.Playerto the output file is a thoughtful change that ensures the generated player enumeration file aligns with the project's namespace structure. This enhances the consistency and maintainability of the codebase.Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (3)
- 1-2: The addition of the
Nhl.Api.Servicesnamespace is appropriate for the context of this file, ensuring that all necessary services are correctly referenced. This change supports the introduction of new service dependencies in theNhlPlayerApiclass.- 11-16: Renaming
_nhlCmsHttpClientto_nhlStaticAssetsApiHttpClientand adding_nhlTeamServiceimproves the clarity and specificity of the variable names, making the code more readable and maintainable. It's also good practice to explicitly declare service dependencies, which aids in understanding the class's responsibilities and dependencies.- 127-127: The validation for
seasonYear's format across various methods is consistent and ensures that the input adheres to the expected format. This is a good practice for maintaining data integrity and preventing errors due to incorrect input formats. However, consider abstracting this validation logic into a separate method or service to avoid code duplication and enhance maintainability.+ private void ValidateSeasonYearFormat(string seasonYear) + { + if (seasonYear?.Length != 8) + { + throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); + } + }This refactoring would centralize the validation logic, making it easier to update or extend in the future.
Also applies to: 150-150, 173-173, 196-196
Nhl.Api.Tests/PlayerTests.cs (16)
- 11-14: The use of
[DataRow]attributes for parameterized testing is a good practice, ensuring that theSearchAllPlayersAsyncmethod is tested with various inputs. This approach enhances test coverage by validating the method's behavior with different player names and expected player numbers.- 15-42: The test method
SearchAllPlayersAsync_Returns_Valid_Resultsis well-structured, following the Arrange-Act-Assert pattern. However, consider adding more detailed comments explaining the purpose of each assertion, especially within theswitchstatement. This will improve the readability and maintainability of the test code by making it clear why each assertion is necessary and what specific aspect of the method's output it verifies.- 74-145: The test method
SearchAllActivePlayersAsync_Returns_Valid_Informationeffectively uses parameterized testing to validate the functionality ofSearchAllActivePlayersAsyncwith various player queries. The assertions within theswitchstatement are comprehensive, covering multiple aspects of the player search result. To further enhance the test, consider adding negative test cases or edge cases, such as querying for a non-existent player, to ensure the method handles such scenarios gracefully.- 150-193: The method
GetPlayerSeasonGameLogsBySeasonAndGameTypeAsync_Test_Player_Enumdemonstrates good testing practices by using parameterized inputs to cover different scenarios. The assertions are thorough, checking not only the presence of data but also specific attributes within the data. To further improve, consider adding assertions that verify the correctness of the data against known values or expected results, where feasible. This would strengthen the test's ability to catch potential inaccuracies in the method's implementation.- 198-209: The test
GetPlayerSeasonGameLogsBySeasonAndGameTypeAsync_Test_PlayerIdeffectively validates the functionality for retrieving player season game logs by player ID. The assertion on the specific count of player game logs (Assert.AreEqual(82, playerSeasonGameLog.PlayerGameLogs.Count);) is a strong validation point. However, ensure that the test data (player ID and season) used in this test is stable and unlikely to change, as relying on specific counts can make the test fragile if the underlying data changes.- 214-257: The method
GetGoalieSeasonGameLogsBySeasonAndGameTypeAsync_Test_PlayerEnumis well-structured and follows best practices for parameterized testing. The assertions comprehensively validate the presence and non-nullity of various attributes in the goalie game logs. To enhance the test, consider including assertions that verify the accuracy of specific attributes (e.g., game date, team abbreviation) against known values. This would help ensure the method not only returns data but returns correct and expected data.- 381-412: The test
GetGoalieInformationAsync_Test_PlayerId_Returns_Valid_Informationeffectively validates the retrieval of goalie information by player ID. The comprehensive set of assertions ensures that all expected attributes of the goalie profile are not null. To further improve the test, consider adding assertions that verify the correctness of certain attributes (e.g., team logo URL, birth country) against expected values. This would provide a stronger guarantee that the method not only returns data but returns accurate data.- 507-538: The method
GetPlayerInformationAsync_Test_PlayerEnumdemonstrates a thorough approach to testing the retrieval of player information using player enums. The extensive set of assertions covers a wide range of attributes, ensuring the method's output is comprehensive. To enhance the test, consider verifying the accuracy of key attributes (e.g., birth date, team abbreviation) against known values for the players being tested. This would add an additional layer of validation to ensure the method's correctness.- 627-638: The test
TestSearchAllPlayersNoResultsAsynccorrectly handles the scenario where no players are found, asserting both the non-nullity of the results and the expected count of zero. This test is important for validating the method's behavior in edge cases and ensuring graceful handling of no results found. The test is well-implemented and covers the necessary assertions for this scenario.- 643-654: The test
TestSearchAllActivePlayersNoResultsAsynceffectively validates the scenario where no active players are found, with appropriate assertions for the non-nullity of results and a count of zero. This consistency in testing both all players and active players for no results scenarios is commendable, ensuring robustness in the API's search functionalities.- 658-672: The test
TestDownloadPlayerHeadshotImageAsyncis well-structured, using parameterized inputs to test the download functionality for player headshot images across different season years. The assertions ensure that an image is returned and that its size is reasonable (greater than 5000 bytes), indicating a successful download. To further improve, consider verifying the image content type (e.g., JPEG, PNG) to ensure the downloaded file is indeed an image.- 693-704: The test
GetPlayerHeadshotImageAsync_Test_Download_PlayerHeadshotImage_With_InvalidIdcorrectly anticipates aJsonReaderExceptionwhen attempting to download a headshot image with an invalid player ID. While testing for exceptions is important, relying on aJsonReaderExceptionmight indicate an underlying issue in error handling within theGetPlayerHeadshotImageAsyncmethod. Ideally, the method should catch such exceptions and throw a more specific exception related to the domain (e.g.,PlayerNotFoundException). Consider improving the error handling in the method to provide clearer feedback to the caller.- 707-734: The test
GetLiveGameFeedPlayerShiftsAsync_Test_Get_Player_Informationeffectively validates the retrieval of live game feed player shifts information. The assertions cover a wide range of attributes, ensuring the method returns detailed and accurate data. To enhance the test, consider adding negative test cases, such as requesting shifts for a game that does not exist, to ensure the method handles error scenarios appropriately.- 737-748: The test
GetLiveGameFeedPlayerShiftsAsync_Test_Is_Player_Information_Emptycorrectly handles the scenario of an invalid game ID, asserting that no player shifts information is returned. This test is important for validating the method's behavior in edge cases and ensuring graceful handling of invalid inputs. The implementation is straightforward and covers the necessary assertions for this scenario.- 751-775: The test
GetPlayerSpotlightAsync_Test_Get_Player_Spotlights_When_Season_Activeintroduces a conditional assertion based on the season's active status. While this approach is innovative, it introduces variability into the test outcome based on external factors (season status). Consider separating the test into two: one that assumes an active season and another that handles the inactive season scenario. This separation would make the test outcomes more predictable and easier to debug.- 780-791: The test
GetAllPlayersAsync_Returns_All_Playerseffectively validates that the method returns a significant number of players, indicating a successful retrieval. However, relying on a specific count threshold (Assert.IsTrue(players.Count > 22000);) can make the test fragile to changes in the underlying data. Consider focusing on the non-nullity and non-emptiness of the returned list as the primary assertions, while treating specific count thresholds as secondary or informational checks.Nhl.Api/Src/StatisticsApi/NhlStatisticsApi.cs (5)
- 12-12: The addition of the
NhlEApiHttpClientinstance_nhlEApiWebHttpClientis consistent with the existing pattern of using static readonly instances for HTTP clients within the class. However, consider the implications of using static instances for HTTP clients, especially regarding scalability and testability. Using dependency injection for these clients could offer more flexibility and make unit testing easier.- 548-578: The method
GetPlayerStatisticsBySeasonAndFilterExpressionAsyncintroduces a new way to retrieve player statistics with a filter and sorting capability. The validation of input parameters (seasonYearandexpressionPlayerFilter) and the handling oflimitandoffsetStartare correctly implemented. However, ensure that theexpressionPlayerFilter.IsValidExpressionproperty is thoroughly tested to prevent potential runtime errors or unexpected behavior when building the endpoint URL.- 590-620: Similar to the previous comment, the method
GetGoalieStatisticsBySeasonAndFilterExpressionAsynccorrectly implements input validation and constructs the request endpoint with filtering and sorting capabilities. Again, ensure that theexpressionGoalieFilter.IsValidExpressionproperty is thoroughly tested. Additionally, consider abstracting the common logic between this method andGetPlayerStatisticsBySeasonAndFilterExpressionAsyncto reduce code duplication.- 621-629: The
ValidateSeasonYearmethod is a good practice for reusing validation logic across multiple methods. However, consider making this method public if it's useful beyond this class or moving it to a utility class if it's used across multiple classes. This change could improve code maintainability and reduce duplication.- 629-629: The validation that the season year must be 8 digits in length is a specific business rule. Ensure that this rule is consistently applied across all relevant parts of the application to maintain data integrity.
Nhl.Api/Src/Api/NhlApi.cs (2)
- 785-798: The method
GetPlayerStatisticsBySeasonAndFilterExpressionAsyncintroduces a flexible way to query player statistics with various filters and sorting options. However, there are a few points to consider:
- Performance: Ensure that the underlying implementation in
_nhlStatisticsApi.GetPlayerStatisticsBySeasonAndFilterExpressionAsyncefficiently handles potentially large datasets, especially whenlimitis set to-1(no limit). Consider implementing pagination or a reasonable default limit if not already done.- Validation: Validate the
seasonYearformat and ensure thatexpressionPlayerFilterandplayerStatisticsFilterToSortByare not null or invalid to prevent runtime errors.- Documentation: The summary mentions "face off percentage, points per game, overtime goals, short handed points, power play points, shooting percentage, shots, time on ice per game and more" as part of the results. Ensure that the documentation accurately reflects the data returned by this method and consider providing examples of
expressionPlayerFilterusage for clarity.Consider adding performance considerations, input validation, and enhancing documentation for clarity and robustness.
- 800-813: Similar to the previous method,
GetGoalieStatisticsBySeasonAndFilterExpressionAsyncprovides a detailed querying mechanism for goalie statistics. The same considerations apply:
- Performance: Verify the performance for large result sets and the handling of the
-1limit. Implementing efficient data retrieval and pagination could be beneficial.- Validation: Ensure input validation for
seasonYear,expressionGoalieFilter, andgoalieStatisticsFilterToSortByto avoid unexpected errors.- Documentation: As with player statistics, ensure the documentation for goalie statistics accurately reflects the returned data and provides clear usage examples for
expressionGoalieFilter.Apply similar refinements as suggested for the player statistics method to ensure consistency, performance, and usability.
Comment on lines
61
to
83
| /// Returns the NHL player's head shot image by season year | ||
| /// </summary> | ||
| /// <param name="player">An NHL player id, Example: 8478402 - Connor McDavid, see <see cref="PlayerEnum"/> for more information on NHL players</param> | ||
| /// <param name="playerHeadshotImageSize">The size of the head shot image, see <see cref="PlayerHeadshotImageSize"/> for more information </param> | ||
| /// <param name="seasonYear">The season year parameter for determining the season for the season, <see cref="SeasonYear"/> for all available seasons</param> | ||
| /// <param name="cancellationToken"> A cancellation token that can be used by other objects or threads to receive notice of cancellation</param> | ||
| /// <returns>A URI endpoint with the image of an NHL player head shot image</returns> | ||
| public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, PlayerHeadshotImageSize playerHeadshotImageSize = PlayerHeadshotImageSize.Small, CancellationToken cancellationToken = default) | ||
| public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, string seasonYear, CancellationToken cancellationToken = default) | ||
| { | ||
| return playerHeadshotImageSize switch | ||
| if (seasonYear?.Length != 8) | ||
| { | ||
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | ||
| } | ||
|
|
||
| var playerInformation = await GetPlayerInformationAsync(player, cancellationToken); | ||
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear)).TeamName.Default; | ||
| if (string.IsNullOrWhiteSpace(teamName)) | ||
| { | ||
| PlayerHeadshotImageSize.Small => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}.png", cancellationToken), | ||
| PlayerHeadshotImageSize.Medium => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}@2x.png", cancellationToken), | ||
| PlayerHeadshotImageSize.Large => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}@3x.png", cancellationToken), | ||
| _ => null, | ||
| }; | ||
| return Array.Empty<byte>(); | ||
| } | ||
|
|
||
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | ||
|
|
||
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{(int)player}.png", cancellationToken); |
There was a problem hiding this comment.
The method GetPlayerHeadshotImageAsync has been updated to accept seasonYear instead of playerHeadshotImageSize, aligning with the objective of retrieving player headshots by season. However, the return type remains byte[], which contradicts the expectation set by previous comments for it to return a Uri. This discrepancy needs resolution for consistency and clarity in the API's interface.
- public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, string seasonYear, CancellationToken cancellationToken = default)
+ public async Task<Uri> GetPlayerHeadshotImageAsync(PlayerEnum player, string seasonYear, CancellationToken cancellationToken = default)Additionally, ensure the seasonYear format is validated and properly handled across all methods accepting this parameter.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| /// Returns the NHL player's head shot image by season year | |
| /// </summary> | |
| /// <param name="player">An NHL player id, Example: 8478402 - Connor McDavid, see <see cref="PlayerEnum"/> for more information on NHL players</param> | |
| /// <param name="playerHeadshotImageSize">The size of the head shot image, see <see cref="PlayerHeadshotImageSize"/> for more information </param> | |
| /// <param name="seasonYear">The season year parameter for determining the season for the season, <see cref="SeasonYear"/> for all available seasons</param> | |
| /// <param name="cancellationToken"> A cancellation token that can be used by other objects or threads to receive notice of cancellation</param> | |
| /// <returns>A URI endpoint with the image of an NHL player head shot image</returns> | |
| public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, PlayerHeadshotImageSize playerHeadshotImageSize = PlayerHeadshotImageSize.Small, CancellationToken cancellationToken = default) | |
| public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, string seasonYear, CancellationToken cancellationToken = default) | |
| { | |
| return playerHeadshotImageSize switch | |
| if (seasonYear?.Length != 8) | |
| { | |
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | |
| } | |
| var playerInformation = await GetPlayerInformationAsync(player, cancellationToken); | |
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear)).TeamName.Default; | |
| if (string.IsNullOrWhiteSpace(teamName)) | |
| { | |
| PlayerHeadshotImageSize.Small => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}.png", cancellationToken), | |
| PlayerHeadshotImageSize.Medium => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}@2x.png", cancellationToken), | |
| PlayerHeadshotImageSize.Large => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}@3x.png", cancellationToken), | |
| _ => null, | |
| }; | |
| return Array.Empty<byte>(); | |
| } | |
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | |
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{(int)player}.png", cancellationToken); | |
| public async Task<Uri> GetPlayerHeadshotImageAsync(PlayerEnum player, string seasonYear, CancellationToken cancellationToken = default) | |
| { | |
| if (seasonYear?.Length != 8) | |
| { | |
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | |
| } | |
| var playerInformation = await GetPlayerInformationAsync(player, cancellationToken); | |
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear)).TeamName.Default; | |
| if (string.IsNullOrWhiteSpace(teamName)) | |
| { | |
| return Array.Empty<byte>(); | |
| } | |
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | |
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{(int)player}.png", cancellationToken); |
Comment on lines
87
to
109
| /// Returns the NHL player's head shot image by season year | ||
| /// </summary> | ||
| /// <param name="playerId">An NHL player id, Example: 8478402 - Connor McDavid</param> | ||
| /// <param name="playerHeadshotImageSize">The size of the head shot image, see <see cref="PlayerHeadshotImageSize"/> for more information </param> | ||
| /// <param name="seasonYear">The season year parameter for determining the season for the season, <see cref="SeasonYear"/> for all available seasons</param> | ||
| /// <param name="cancellationToken"> A cancellation token that can be used by other objects or threads to receive notice of cancellation</param> | ||
| /// <returns>A URI endpoint with the image of an NHL player head shot image</returns> | ||
| public async Task<byte[]> GetPlayerHeadshotImageAsync(int playerId, PlayerHeadshotImageSize playerHeadshotImageSize = PlayerHeadshotImageSize.Small, CancellationToken cancellationToken = default) | ||
| public async Task<byte[]> GetPlayerHeadshotImageAsync(int playerId, string seasonYear, CancellationToken cancellationToken = default) | ||
| { | ||
| return playerHeadshotImageSize switch | ||
| if (seasonYear?.Length != 8) | ||
| { | ||
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | ||
| } | ||
|
|
||
| var playerInformation = await GetPlayerInformationAsync(playerId, cancellationToken); | ||
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear)).TeamName.Default; | ||
| if (string.IsNullOrWhiteSpace(teamName)) | ||
| { | ||
| PlayerHeadshotImageSize.Small => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{playerId}.png", cancellationToken), | ||
| PlayerHeadshotImageSize.Medium => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{playerId}@2x.png", cancellationToken), | ||
| PlayerHeadshotImageSize.Large => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{playerId}@3x.png", cancellationToken), | ||
| _ => null, | ||
| }; | ||
| return Array.Empty<byte>(); | ||
| } | ||
|
|
||
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | ||
|
|
||
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{playerId}.png", cancellationToken); |
There was a problem hiding this comment.
Similar to the previous comment, the GetPlayerHeadshotImageAsync method for retrieving headshot images by player ID has been correctly updated to use seasonYear. However, the return type issue remains, where the method is expected to return a URI endpoint but currently returns a byte[].
- public async Task<byte[]> GetPlayerHeadshotImageAsync(int playerId, string seasonYear, CancellationToken cancellationToken = default)
+ public async Task<Uri> GetPlayerHeadshotImageAsync(int playerId, string seasonYear, CancellationToken cancellationToken = default)Ensure the return type is corrected across all relevant methods to align with the intended functionality and the API's documentation.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| /// Returns the NHL player's head shot image by season year | |
| /// </summary> | |
| /// <param name="playerId">An NHL player id, Example: 8478402 - Connor McDavid</param> | |
| /// <param name="playerHeadshotImageSize">The size of the head shot image, see <see cref="PlayerHeadshotImageSize"/> for more information </param> | |
| /// <param name="seasonYear">The season year parameter for determining the season for the season, <see cref="SeasonYear"/> for all available seasons</param> | |
| /// <param name="cancellationToken"> A cancellation token that can be used by other objects or threads to receive notice of cancellation</param> | |
| /// <returns>A URI endpoint with the image of an NHL player head shot image</returns> | |
| public async Task<byte[]> GetPlayerHeadshotImageAsync(int playerId, PlayerHeadshotImageSize playerHeadshotImageSize = PlayerHeadshotImageSize.Small, CancellationToken cancellationToken = default) | |
| public async Task<byte[]> GetPlayerHeadshotImageAsync(int playerId, string seasonYear, CancellationToken cancellationToken = default) | |
| { | |
| return playerHeadshotImageSize switch | |
| if (seasonYear?.Length != 8) | |
| { | |
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | |
| } | |
| var playerInformation = await GetPlayerInformationAsync(playerId, cancellationToken); | |
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear)).TeamName.Default; | |
| if (string.IsNullOrWhiteSpace(teamName)) | |
| { | |
| PlayerHeadshotImageSize.Small => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{playerId}.png", cancellationToken), | |
| PlayerHeadshotImageSize.Medium => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{playerId}@2x.png", cancellationToken), | |
| PlayerHeadshotImageSize.Large => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{playerId}@3x.png", cancellationToken), | |
| _ => null, | |
| }; | |
| return Array.Empty<byte>(); | |
| } | |
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | |
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{playerId}.png", cancellationToken); | |
| /// Returns the NHL player's head shot image by season year | |
| /// </summary> | |
| /// <param name="playerId">An NHL player id, Example: 8478402 - Connor McDavid</param> | |
| /// <param name="seasonYear">The season year parameter for determining the season for the season, <see cref="SeasonYear"/> for all available seasons</param> | |
| /// <param name="cancellationToken"> A cancellation token that can be used by other objects or threads to receive notice of cancellation</param> | |
| /// <returns>A URI endpoint with the image of an NHL player head shot image</returns> | |
| public async Task<Uri> GetPlayerHeadshotImageAsync(int playerId, string seasonYear, CancellationToken cancellationToken = default) | |
| { | |
| if (seasonYear?.Length != 8) | |
| { | |
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | |
| } | |
| var playerInformation = await GetPlayerInformationAsync(playerId, cancellationToken); | |
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear)).TeamName.Default; | |
| if (string.IsNullOrWhiteSpace(teamName)) | |
| { | |
| return Array.Empty<byte>(); | |
| } | |
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | |
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{playerId}.png", cancellationToken); |
Nhl.Api.Tests/PlayerTests.cs
Outdated
Comment on lines
795
to
800
| [TestMethodWithRetry(RetryCount = 25)] | ||
| public async Task PlayerEnumFileGeneratorHelper_Returns_Valid_Content() | ||
| { | ||
| // Arrange | ||
| await PlayerEnumFileGeneratorHelper.UpdatePlayerEnumToFile(string.Empty); | ||
| } |
There was a problem hiding this comment.
The test PlayerEnumFileGeneratorHelper_Returns_Valid_Content lacks assertions, making it unclear what aspect of the UpdatePlayerEnumToFile method's behavior is being tested. For a test to be effective, it should include assertions that validate the expected outcome of the method under test. Consider adding assertions that verify the content of the file after the update operation, ensuring that the method functions as intended.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- .github/workflows/develop-build.yml (1 hunks)
- .github/workflows/master-build-daily.yml (1 hunks)
- .github/workflows/master-build.yml (1 hunks)
- .github/workflows/publish-nuget.yml (1 hunks)
Additional comments: 4
.github/workflows/master-build-daily.yml (1)
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-7]
The workflow now exclusively uses .NET 8. Ensure that all projects within the repository are compatible with .NET 8 to avoid build failures.
.github/workflows/publish-nuget.yml (1)
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-7]
The workflow now exclusively uses .NET 8 for building and publishing NuGet packages. Confirm that the NuGet package is fully compatible with .NET 8 to prevent any compatibility issues for consumers.
Verification successful
The verification process has successfully confirmed that the NuGet package (or project) specifies .NET 8 compatibility, as indicated by the presence of
<TargetFramework>net8.0</TargetFramework>in a.csprojfile. This aligns with the changes made in the GitHub Actions workflow to use .NET 8 for building and publishing NuGet packages.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the NuGet package specifies .NET 8 compatibility rg '<TargetFramework>net8.0</TargetFramework>' --type csprojLength of output: 93
Script:
#!/bin/bash # Corrected script to verify if the NuGet package specifies .NET 8 compatibility by searching within .csproj files rg '<TargetFramework>net8.0</TargetFramework>' -g '*.csproj'Length of output: 141
.github/workflows/master-build.yml (1)
- 8-13: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-7]
The workflow now exclusively uses .NET 8 for master branch builds. It's crucial to ensure that the code coverage tools and Codecov integration are fully compatible with .NET 8 to maintain accurate coverage reporting.
.github/workflows/develop-build.yml (1)
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-7]
The workflow now exclusively uses .NET 8 for develop branch builds. Similar to the master branch workflow, ensure that the code coverage tools and Codecov integration are fully compatible with .NET 8.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .github/workflows/develop-build.yml (1 hunks)
- .github/workflows/master-build-daily.yml (1 hunks)
- .github/workflows/master-build.yml (1 hunks)
- .github/workflows/publish-nuget.yml (1 hunks)
- Nhl.Api.Tests/PlayerTests.cs (5 hunks)
- Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (7 hunks)
Files skipped from review as they are similar to previous changes (5)
- .github/workflows/develop-build.yml
- .github/workflows/master-build-daily.yml
- .github/workflows/master-build.yml
- .github/workflows/publish-nuget.yml
- Nhl.Api.Tests/PlayerTests.cs
Additional comments: 6
Nhl.Api/Src/PlayerApi/NhlPlayerApi.cs (6)
- 2-2: Adding the
Nhl.Api.Servicesnamespace is appropriate for accessing services within the Nhl.Api project. This change aligns with the PR's objective of enhancing project structure and code quality.- 14-14: Renaming
_nhlCmsHttpClientto_nhlStaticAssetsApiHttpClientimproves clarity and better reflects the purpose of the HttpClient instance. This change is consistent with the PR's goal of refining method parameters and improving code readability.- 16-16: The addition of
_nhlTeamServicedeclaration is a positive change, indicating an improvement in dependency management as outlined in the PR objectives. It's important to ensure that this service is utilized effectively throughout the class.- 87-109: The changes in the overloaded
GetPlayerHeadshotImageAsyncmethod for handlingint playerIdparameter are consistent with the previous comment. It's good to see consistency in handlingseasonYearacross different method overloads. The same note aboutseasonYearformat validation applies here.- 124-130: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [127-150]
The updates to parameter validation in
GetPlayerSeasonGameLogsBySeasonAndGameTypeAsyncmethods for bothPlayerEnumandint playerIdversions are in line with the PR's objective of enhanced validation and error handling. The consistent use ofseasonYearformat validation across methods is noted. However, consider abstracting common validation logic to reduce code duplication.Consider abstracting common validation logic for
seasonYearto a private method to reduce code duplication and improve maintainability.
- 170-176: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [173-196]
Similar to the previous comment, the updates in
GetGoalieSeasonGameLogsBySeasonAndGameTypeAsyncmethods for bothPlayerEnumandint playerIdversions demonstrate an effort to enhance validation and error handling. The repetition ofseasonYearformat validation suggests an opportunity for refactoring.As mentioned earlier, abstracting common validation logic for
seasonYearcould enhance code maintainability and reduce duplication.
Comment on lines
+61
to
+83
| /// Returns the NHL player's head shot image by season year | ||
| /// </summary> | ||
| /// <param name="player">An NHL player id, Example: 8478402 - Connor McDavid, see <see cref="PlayerEnum"/> for more information on NHL players</param> | ||
| /// <param name="playerHeadshotImageSize">The size of the head shot image, see <see cref="PlayerHeadshotImageSize"/> for more information </param> | ||
| /// <param name="seasonYear">The season year parameter for determining the season for the season, <see cref="SeasonYear"/> for all available seasons</param> | ||
| /// <param name="cancellationToken"> A cancellation token that can be used by other objects or threads to receive notice of cancellation</param> | ||
| /// <returns>A URI endpoint with the image of an NHL player head shot image</returns> | ||
| public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, PlayerHeadshotImageSize playerHeadshotImageSize = PlayerHeadshotImageSize.Small, CancellationToken cancellationToken = default) | ||
| public async Task<byte[]> GetPlayerHeadshotImageAsync(PlayerEnum player, string seasonYear, CancellationToken cancellationToken = default) | ||
| { | ||
| return playerHeadshotImageSize switch | ||
| if (seasonYear?.Length != 8) | ||
| { | ||
| throw new ArgumentException($"The {nameof(seasonYear)} parameter must be in the format of yyyyyyyy, example: 20232024", nameof(seasonYear)); | ||
| } | ||
|
|
||
| var playerInformation = await GetPlayerInformationAsync(player, cancellationToken); | ||
| var teamName = playerInformation.SeasonTotals.FirstOrDefault(x => x.Season == int.Parse(seasonYear))?.TeamName?.Default; | ||
| if (string.IsNullOrWhiteSpace(teamName)) | ||
| { | ||
| PlayerHeadshotImageSize.Small => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}.png", cancellationToken), | ||
| PlayerHeadshotImageSize.Medium => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}@2x.png", cancellationToken), | ||
| PlayerHeadshotImageSize.Large => await _nhlCmsHttpClient.GetByteArrayAsync($"images/headshots/current/168x168/{(int)player}@3x.png", cancellationToken), | ||
| _ => null, | ||
| }; | ||
| return Array.Empty<byte>(); | ||
| } | ||
|
|
||
| var teamAbbreviation = _nhlTeamService.GetTeamCodeIdentifierByTeamName(teamName); | ||
|
|
||
| return await _nhlStaticAssetsApiHttpClient.GetByteArrayAsync($"mugs/nhl/{seasonYear}/{teamAbbreviation}/{(int)player}.png", cancellationToken); |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results confirm the consistent use of the seasonYear?.Length != 8 validation across various parts of the project, aligning with the review comment's observation. However, while this validation checks for the correct length of the seasonYear string, it does not ensure the content is a valid season year format beyond the length check. This means the validation is consistent but may not be fully robust in catching all invalid formats, as it does not verify the content/format beyond the length. This observation supports the review comment's suggestion to ensure the robustness of the seasonYear format validation.
Analysis chain
The modifications to GetPlayerHeadshotImageAsync method, including the addition of seasonYear parameter and the updated logic to handle image retrieval based on the season year, align with the PR's objective of enhancing functionality and clarity. However, ensure that the seasonYear format validation (seasonYear?.Length != 8) is robust enough to catch all invalid formats.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the seasonYear format validation is robust across all usages in the project.
rg --type cs "seasonYear\?.Length != 8"
Length of output: 1213
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/codeql-analysis.yml (3 hunks)
- Nhl.Api.Tests/TeamTests.cs (2 hunks)
- Nhl.Api/README.md (17 hunks)
Files not summarized due to errors (1)
- Nhl.Api/README.md: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- Nhl.Api/NhlStatisticsApi.cs (no review received)
Additional comments: 4
.github/workflows/codeql-analysis.yml (3)
- 46-46: The update of the CodeQL action to
v3for initialization is a good practice as it ensures the workflow benefits from the latest features, security patches, and performance improvements provided by GitHub. Always ensure to test the workflow execution to confirm there are no breaking changes introduced by the version update.- 57-57: The update of the CodeQL autobuild action to
v3is approved. This change is consistent with the update to the initialization step and helps maintain compatibility across the workflow. As with any major version update, verify the autobuild process in the CI environment to ensure it completes successfully without errors.- 71-71: Updating the CodeQL analysis action to
v3aligns with the updates to the other CodeQL actions in this workflow. This consistency is crucial for the smooth operation of the CodeQL scanning process. Remember to review the analysis results post-update to ensure the expected behavior of the security scanning.Nhl.Api.Tests/TeamTests.cs (1)
- 494-534: The addition of the
TeamNames_Ensure_Each_TeamAbbreviation_Is_Correcttest method is a valuable enhancement to the test suite. It verifies the correctness of team abbreviations, which is crucial for data integrity and application functionality. However, consider the following improvements for better test maintainability and clarity:
- Parameterized Testing: You've correctly used
DataRowattributes to test multiple inputs. This approach keeps the test code DRY and makes it easy to add new test cases.- Assert Message: Adding a message to the
Assertstatements can provide more context when a test fails, making it easier to diagnose issues. For example,Assert.AreEqual(teamAbbreviation.Length, 3, $"The abbreviation for {teamName} did not meet the expected length.");- Service Mocking: If
NhlTeamServiceinteracts with external systems or APIs, consider mocking these dependencies to ensure the test remains focused on the logic it intends to test and is not affected by external factors.Overall, the test method is well-structured and serves its purpose effectively.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
seasonYearparameter in league API methods to better handle null values.