Fix/xpath player transfers youth clubs #78
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/players/test_players_profile.py (2)
Line range hint
183-183: Consider documenting schema variations.The field renaming is consistently applied here. However, since this test case represents a different player status (retired) with a unique schema structure, consider adding a comment explaining these variations to improve maintainability.
+ # Schema for retired players differs from active players + # - Excludes current club details (joined, contractExpires) + # - Includes retirement-specific fields (retiredSince, lastClubID, etc.) expected_schema = Schema(
Line range hint
1-248: Consider implementing shared schema definitions.The test file contains multiple similar schema definitions with variations. To improve maintainability and reduce duplication, consider:
- Creating base schemas for common fields
- Extending base schemas for specific cases (active vs. retired players)
- Documenting the relationship between XPath changes and schema updates
Example approach:
BASE_PLAYER_SCHEMA = { "id": And(str, len_greater_than_0), "url": And(str, len_greater_than_0), # ... other common fields } ACTIVE_PLAYER_SCHEMA = { **BASE_PLAYER_SCHEMA, "club": { "id": And(str, len_greater_than_0), # ... active player specific fields } } RETIRED_PLAYER_SCHEMA = { **BASE_PLAYER_SCHEMA, "retiredSince": And(str, len_greater_than_0), # ... retired player specific fields }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/utils/xpath.py (1 hunks)
- tests/players/test_players_profile.py (1 hunks)
🧰 Additional context used
🪛 Ruff
app/utils/xpath.py
74-74: Line too long (142 > 120)
(E501)
🔇 Additional comments (3)
tests/players/test_players_profile.py (2)
Line range hint
134-138: Verify contract option field requirements across different scenarios.The
contractOptionfield has been added only to this test case. Consider:
- Should this field be optional in other test cases?
- Are there specific conditions that determine when this field should be present?
#!/bin/bash # Description: Search for any other instances of contractOption in the codebase # This will help understand the field's usage pattern rg "contractOption" -A 5 -B 5 # Search for contract-related fields in the codebase ast-grep --pattern 'contract[A-Z][a-zA-Z]*'
71-71:⚠️ Potential issueVerify consistency of field renaming across all test cases.
The field has been renamed from
nameInHomeCountrytofullNamehere, buttest_get_player_profile_28003still uses the old field name. This inconsistency should be addressed.app/utils/xpath.py (1)
74-74: LGTM! More robust XPath for youth clubs.The new XPath expression is more specific and robust:
- Uses class-based selection instead of data-viewport attribute
- Adds text content validation for "Youth"
- Targets the specific content div
Let's verify the XPath expression works as expected:
✅ Verification successful
Based on the search results, I can now generate the final response:
XPath expression verified and appears to be correctly implemented
The verification shows:
- The
YOUTH_CLUBSXPath is used consistently in the codebase- It's properly integrated into the transfers service (
app/services/players/transfers.py)- The data is correctly mapped to the
youthClubsfield in the response- Test coverage exists (
tests/players/test_players_transfers.py) expecting theyouthClubsfield as a listThe new XPath expression is more specific and robust, targeting the correct data structure while maintaining consistency with the codebase's architecture.
Note: I'm not including the line length issue from the original review as that would be a separate concern and should be addressed in a different comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any other occurrences of youth-related XPath expressions # to ensure consistency and avoid duplicates # Test 1: Search for youth-related XPath expressions rg -i "youth|jugend" --type py # Test 2: Search for similar box class usage ast-grep --pattern $'//div[@class=\'box tm-player-additional-data\']'Length of output: 918
🧰 Tools
🪛 Ruff
74-74: Line too long (142 > 120)
(E501)
app/utils/xpath.py
Outdated
| @@ -71,7 +71,7 @@ class MarketValue: | |||
| RANKINGS_POSITIONS = "//span[contains(@class, 'quick-fact__content--large')]//text()" | |||
|
|
|||
| class Transfers: | |||
| YOUTH_CLUBS = "//div[@data-viewport='Jugendvereine']//div//text()" | |||
| YOUTH_CLUBS = "//div[@class='box tm-player-additional-data'][descendant::*[contains(text(), 'Youth')]]//div[@class='content']//text()" | |||
There was a problem hiding this comment.
Fix line length to comply with PEP 8.
The line exceeds the maximum length of 120 characters.
Split the XPath expression across multiple lines:
- YOUTH_CLUBS = "//div[@class='box tm-player-additional-data'][descendant::*[contains(text(), 'Youth')]]//div[@class='content']//text()"
+ YOUTH_CLUBS = (
+ "//div[@class='box tm-player-additional-data']"
+ "[descendant::*[contains(text(), 'Youth')]]"
+ "//div[@class='content']//text()"
+ )📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| YOUTH_CLUBS = "//div[@class='box tm-player-additional-data'][descendant::*[contains(text(), 'Youth')]]//div[@class='content']//text()" | |
| YOUTH_CLUBS = ( | |
| "//div[@class='box tm-player-additional-data']" | |
| "[descendant::*[contains(text(), 'Youth')]]" | |
| "//div[@class='content']//text()" | |
| ) |
🧰 Tools
🪛 Ruff
74-74: Line too long (142 > 120)
(E501)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
nameInHomeCountrytofullNamein player profile tests.contractOptionfield in the expected schema for specific player profile tests.