-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added squad numbers API #62
Conversation
Someone is attempting to deploy a commit to a Personal Account owned by @felipeall on Vercel. @felipeall first needs to authorize it. |
WalkthroughThe recent update enhances the application by enabling users to access player squad and jersey numbers from Transfermarkt. This functionality is made possible through a new API endpoint for retrieving squad numbers and the introduction of service classes dedicated to fetching and parsing squad and jersey numbers data. Additionally, XPath expressions specific to squad and jersey numbers have been incorporated into the utility module for efficient data extraction. 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 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- app/api/endpoints/players.py (2 hunks)
- app/services/players/squadnumbers.py (1 hunks)
- app/utils/xpath.py (1 hunks)
Additional comments: 4
app/api/endpoints/players.py (2)
- 12-12: The import statement for
TransfermarktPlayerSquadNumbers
is correctly added to support the new endpoint functionality.- 45-49: The new endpoint
/squadnumbers
is well-defined and follows the existing pattern of the API design. However, consider adding a brief docstring to theget_player_squad_numbers
function to describe its purpose and parameters for consistency with best practices and maintainability.app/services/players/squadnumbers.py (1)
- 28-50: The private method
__parse_player_squadnumbers
is well-implemented, using XPath expressions to parse the squad numbers data. It's good practice to keep such parsing logic encapsulated within a private method. However, ensure that the XPath expressions used are robust and tested across different player pages to handle any variations in the page structure.app/utils/xpath.py (1)
- 12-17: The
SquadNumbers
class is correctly added within thePlayers
class, providing a centralized location for XPath expressions related to squad numbers. Ensure that these XPath expressions are accurate and tested across different player pages to handle variations in the HTML structure of the Transfermarkt website.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sthewissen , thanks for your contribuition to the project by opening this PR, I really appreciate it!!
Overall it is pretty good! My biggest concern is how we are going to call it, I feel that "Jersey Number" makes more sense than "Squad Number", at least in the main providers I've seen. What do you think?
If you agree, we can just rename the references from "squad number" to "jersey number" in the variables/classes/docstrings. Also I left some minor comments just to follow the existing codebase standards.
Again, thank you very much for the contribution and please let me know if you have any questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- app/api/endpoints/players.py (2 hunks)
- app/services/players/jersey_numbers.py (1 hunks)
- app/utils/xpath.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/api/endpoints/players.py
- app/utils/xpath.py
Additional comments: 2
app/services/players/jersey_numbers.py (2)
- 9-17: The class docstring provides a clear overview of the class's purpose and its arguments. However, it mentions
URL
as an argument, which is not passed as an argument but is a class attribute with a default value.Consider updating the docstring to clarify that
URL
is a class attribute rather than an argument to avoid confusion.
- 22-27: The
__post_init__
method correctly formats theURL
with theplayer_id
and fetches the page. However, it uses a hard-coded XPath expression (Players.Profile.URL
) to check if the page is found. This approach might not be flexible if the structure of the Transfermarkt website changes.Consider making the XPath expression for checking page availability a class attribute or parameter to increase flexibility and maintainability.
I'm not a Python developer, so please forgive any problems this might have. I needed an API that returns squad numbers for any given player, so I did my best to add it and thought I'd contribute by PRing it back.
Summary by CodeRabbit