Add TransfermarktPlayerInjuries service #49
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate Limit Exceeded@felipeall has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 3 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe overall change involves refactoring and expanding the functionality related to player injuries, search pagination, and data extraction. A new endpoint for player injuries has been added, and the logic for determining the last page number in search results has been generalized across different services. Import statements have been consolidated for clarity, and new XPath expressions have been introduced to support the updated and new features. Changes
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 X ? TipsChat with CodeRabbit Bot (
|
felipeall
force-pushed
the
bjethwan/main
branch
from
157a957
to
206962a
Compare
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- app/api/endpoints/players.py (2 hunks)
- app/services/base.py (2 hunks)
- app/services/clubs/search.py (1 hunks)
- app/services/competitions/search.py (1 hunks)
- app/services/players/injuries.py (1 hunks)
- app/services/players/market_value.py (1 hunks)
- app/services/players/search.py (1 hunks)
- app/services/players/stats.py (1 hunks)
- app/services/players/transfers.py (1 hunks)
- app/utils/xpath.py (2 hunks)
Files skipped from review due to trivial changes (3)
- app/services/players/market_value.py
- app/services/players/stats.py
- app/services/players/transfers.py
Additional comments: 12
app/api/endpoints/players.py (3)
5-8: The import of
TransfermarktPlayerInjuriesis correctly placed and follows the existing import structure.51-55: The new endpoint
get_player_injuriesis correctly defined and follows the established pattern of other endpoints in the file. Ensure that the corresponding service methodget_player_injurieshandles thepage_numberparameter correctly, as it is optional in the endpoint definition.46-47: The change in the
get_player_statsfunction to returnplayer_statsis consistent with the PR's objective to update the function to return player statistics instead of market value.app/services/base.py (3)
12-12: The import statement has been updated to reflect the use of
Paginationinstead ofCommons. This change is consistent with the PR objectives.204-218: The refactoring of the
get_last_page_numbermethod simplifies the page number extraction by iterating over a set of XPaths. Ensure that the XPaths provided inPaginationcover all cases previously handled by the method. Also, verify that the loop does not introduce performance issues when dealing with a large number of pages.204-204: The method signature for
get_last_page_numbernow includes a default value forxpath_base. Verify that all existing calls to this method in the codebase are compatible with this change.Verification successful
The method signature change for
get_last_page_numberinapp/services/base.pyto include a default value forxpath_baseis compatible with all existing calls found in the codebase. The calls inplayers/search.py,competitions/search.py,clubs/search.py, andplayers/injuries.pyare consistent with the updated method signature.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to `get_last_page_number` without arguments to ensure they are still valid. ast-grep --lang python --pattern $'get_last_page_number()'Length of output: 59
Script:
#!/bin/bash # Search for all calls to `get_last_page_number` to ensure they are compatible with the new method signature. ast-grep --lang python --pattern $'get_last_page_number($$$)'Length of output: 62
Script:
#!/bin/bash # Search for the string 'get_last_page_number' to catch any potential dynamic or indirect references to the method. rg 'get_last_page_number'Length of output: 580
app/services/clubs/search.py (2)
- 76-76: The change from
get_search_last_page_numbertoget_last_page_numberaligns with the PR's objective to refactor pagination logic. Ensure that theget_last_page_numbermethod is correctly implemented inTransfermarktBaseand that it accepts the same arguments as the replaced method.Verification successful
The
get_last_page_numbermethod is indeed present in the codebase and is defined withinapp/services/base.py. The method signature indicates that it accepts a single parameter,xpath_base, which is an optional string parameter with a default value of an empty string. This confirms that the method exists and is likely to be the one that has replaced theget_search_last_page_numbermethod in thesearch_clubsfunction withinapp/services/clubs/search.py.Given this information, the change from
get_search_last_page_numbertoget_last_page_numberin thesearch_clubsfunction is verified to be consistent with the method's existence and signature in theTransfermarktBaseclass (or a class that extends it).* 76-76: Verify that all instances of the old `get_search_last_page_number` method have been replaced with the new `get_last_page_number` method across the entire codebase to maintain consistency.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of get_last_page_number in TransfermarktBase ast-grep --lang python --pattern $'class TransfermarktBase { $$$ def get_last_page_number($_) { $$$ } $$$ }'Length of output: 466
Script:
#!/bin/bash # Search for the get_last_page_number method in the entire codebase ast-grep --lang python --pattern $'def get_last_page_number($_) { $$$ }'Length of output: 536
Script:
#!/bin/bash # Correct the pattern syntax and search for the get_last_page_number method in the entire codebase ast-grep --lang python --pattern $'def get_last_page_number($$$) { $$$ }'Length of output: 538
Script:
#!/bin/bash # Attempt to correct the pattern syntax again and search for the get_last_page_number method in the entire codebase ast-grep --lang python --pattern $'def get_last_page_number($_) { $$$ }'Length of output: 536
Script:
#!/bin/bash # Use ripgrep to search for the get_last_page_number method in the codebase rg "def get_last_page_number"Length of output: 117
Verification successful
The shell script executed a search for the string
get_search_last_page_numberacross the codebase and did not produce any output. This suggests that there are no remaining usages of the oldget_search_last_page_numbermethod, and it has likely been replaced by the newget_last_page_numbermethod everywhere it was previously used.Based on this information, it can be concluded that the replacement of the old method with the new one has been done consistently across the entire codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of the old get_search_last_page_number method rg 'get_search_last_page_number' --files-with-matchesLength of output: 53
app/services/players/search.py (1)
- 88-88:
The update fromget_search_last_page_numbertoget_last_page_numberin thesearch_playersmethod aligns with the PR objectives to refactor the pagination logic. Ensure that theget_last_page_numbermethod is correctly implemented inapp/services/base.pyand that its signature matches this usage.app/utils/xpath.py (3)
2-10: The addition of the
Players.Injuriesclass with XPath expressions for injury data looks correct and consistent with the rest of the module.173-175: The addition of the
Paginationclass with XPath expressions for pagination data looks correct and consistent with the rest of the module.1-14: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-175]
Overall, the changes to
app/utils/xpath.pyalign with the PR's objectives and the AI-generated summary. The new XPath expressions for injury and pagination data have been added correctly.
app/services/players/injuries.py
Outdated
| @dataclass | ||
| class TransfermarktPlayerInjuries(TransfermarktBase): | ||
| player_id: str = None | ||
| URL: str = "https://www.transfermarkt.com/player/verletzungen/spieler/{player_id}/plus/1/page/{page_number}" | ||
| page_number: int = 1 | ||
|
|
||
| def __post_init__(self): | ||
| self.URL = self.URL.format(player_id=self.player_id, page_number=self.page_number) | ||
| self.page = self.request_url_page() | ||
| self.raise_exception_if_not_found(xpath=Players.Profile.URL) | ||
|
|
||
| def __parse_player_injuries(self) -> Optional[List[dict]]: | ||
| injuries: ElementTree = self.page.xpath(Players.Injuries.RESULTS) | ||
| player_injuries = [] | ||
|
|
||
| for injury in injuries: | ||
| season = trim(injury.xpath(Players.Injuries.SEASONS)) | ||
| injury_type = trim(injury.xpath(Players.Injuries.INJURY)) | ||
| date_from = trim(injury.xpath(Players.Injuries.FROM)) | ||
| date_until = trim(injury.xpath(Players.Injuries.UNTIL)) | ||
| days = trim(injury.xpath(Players.Injuries.DAYS)) | ||
| games_missed = trim(injury.xpath(Players.Injuries.GAMES_MISSED)) | ||
| games_missed_clubs_urls = injury.xpath(Players.Injuries.GAMES_MISSED_CLUBS_URLS) | ||
| games_missed_clubs_ids = [extract_from_url(club_url) for club_url in games_missed_clubs_urls] | ||
|
|
||
| player_injuries.append( | ||
| { | ||
| "season": season, | ||
| "injury": injury_type, | ||
| "from": date_from, | ||
| "until": date_until, | ||
| "days": days, | ||
| "gamesMissed": games_missed, | ||
| "gamesMissedClubs": games_missed_clubs_ids, | ||
| }, | ||
| ) | ||
|
|
||
| return player_injuries | ||
|
|
||
| def get_player_injuries(self) -> dict: | ||
| self.response["id"] = self.player_id | ||
| self.response["pageNumber"] = self.page_number | ||
| self.response["lastPageNumber"] = self.get_last_page_number() | ||
| self.response["injuries"] = self.__parse_player_injuries() | ||
| self.response["updatedAt"] = datetime.now() | ||
|
|
||
| return clean_response(self.response) |
There was a problem hiding this comment.
The TransfermarktPlayerInjuries class is well-structured and follows the inheritance pattern from TransfermarktBase. The use of utility functions and XPath expressions from other modules is consistent with the application's design. The methods __post_init__, __parse_player_injuries, and get_player_injuries are clearly defined and seem to follow the intended logic for fetching and parsing player injury data.
However, there are a few points to consider:
-
The
player_idattribute is initialized asNoneby default (line 13), which could lead to a malformed URL if not provided. It might be safer to requireplayer_idas a mandatory parameter during initialization to avoid potential runtime errors. -
The
__parse_player_injuriesmethod is prefixed with double underscores, which in Python indicates a name mangling to create a private version of the variable or method. If this method is intended to be used only within this class, this is fine. However, if it is meant to be accessed from outside, consider renaming it with a single underscore to indicate it is a protected member of the class. -
The
get_player_injuriesmethod updates theself.responsedictionary directly (lines 51-55). Ensure thatself.responseis defined in the base class or within this class to avoid anAttributeError. -
The
datetime.now()method used in line 55 does not include timezone information. It's recommended to usedatetime.now(timezone.utc)to have timezone-aware timestamps, which are generally more robust for applications that may run in different time zones or that store data that could be accessed from multiple time zones. -
The
clean_responseutility function is used to return the response (line 57). Ensure that this function is thoroughly tested, especially since it's handling the output of the service which could be critical for the application's functionality. -
Error handling is not visible in the provided hunk. It's important to ensure that there are proper error handling mechanisms in place, especially for network calls and XPath queries which can fail or return unexpected results.
-
The use of XPath queries (lines 23-34) assumes that the structure of the HTML from
transfermarkt.comwill not change. If the website's layout changes, these queries may break. It's advisable to have a fallback mechanism or a monitoring system to alert when the data extraction process fails. -
The method
get_last_page_numberis called without being defined within this class (line 53). It's assumed to be a method from the base classTransfermarktBase. Ensure that this method exists and is correctly implemented in the base class. -
The
extract_from_urlutility function is used to extract club IDs from URLs (line 34). Ensure that this function is robust and can handle any edge cases or unexpected URL formats. -
The
self.request_url_page()method is called in the__post_init__method (line 19). Ensure that this method includes proper exception handling for HTTP errors or connection issues.
Overall, the class seems to be well-integrated with the rest of the application, and the methods are consistent with the PR's objectives. The above points are mainly for ensuring robustness and maintainability of the code.
felipeall
force-pushed
the
bjethwan/main
branch
from
1d507de
to
89e2ed2
Compare
Summary by CodeRabbit
New Features
Improvements
get_player_statsendpoint to return player statistics.Refactor
Documentation
Bug Fixes
Style
Tests
Chores
Revert