refactor: implement pydantic schemas #80
Conversation
|
Warning Rate limit exceeded@felipeall has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (34)
WalkthroughThe changes encompass modifications across several FastAPI endpoint definitions and schema files. Key updates include the addition of 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 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: 22
🧹 Outside diff range and nitpick comments (42)
app/schemas/players/jersey_numbers.py (1)
10-11: Enhance schema documentation and data validation.Consider adding:
- Class documentation explaining the schema and its mixins
- Validation to prevent duplicate season/club combinations
Apply these improvements:
class PlayerJerseyNumbers(TransfermarktBaseModel, IDMixin, AuditMixin): - jersey_numbers: list[JerseyNumber] + """Collection of jersey numbers for a player across different seasons and clubs. + + Inherits: + TransfermarktBaseModel: Base model with common functionality + IDMixin: Adds ID field + AuditMixin: Adds audit fields (created_at, updated_at) + """ + jersey_numbers: list[JerseyNumber] = Field( + min_items=1, + description="List of jersey numbers across seasons" + ) + + @validator("jersey_numbers") + def validate_unique_season_club(cls, v): + """Ensure no duplicate season/club combinations exist.""" + seen = set() + for jn in v: + key = (jn.season, jn.club) + if key in seen: + raise ValueError( + f"Duplicate season/club combination: {jn.season} - {jn.club}" + ) + seen.add(key) + return vapp/schemas/competitions/clubs.py (1)
4-6: Add field constraints and documentation.Consider enhancing this model with:
- Field constraints for the
namefield (e.g., min/max length)- A docstring explaining the purpose of this model
class CompetitionClub(TransfermarktBaseModel, IDMixin): + """Represents a club within a competition context.""" - name: str + name: str = Field( + min_length=1, + max_length=100, + description="The name of the club in the competition" + )app/schemas/players/market_value.py (1)
7-12: Consider adding field constraints and descriptionsWhile the basic structure is good, consider these improvements for better validation and documentation:
class MarketValueHistory(TransfermarktBaseModel): - age: int - date: date - club_id: str - club_name: str - market_value: Optional[int] = None + age: int = Field( + ..., + ge=0, + le=100, + description="Player's age at the time of market value record" + ) + date: date = Field( + ..., + description="Date when the market value was recorded" + ) + club_id: str = Field( + ..., + min_length=1, + description="Unique identifier of the player's club" + ) + club_name: str = Field( + ..., + min_length=1, + description="Name of the player's club" + ) + market_value: Optional[int] = Field( + None, + ge=0, + description="Player's market value in the default currency" + )app/schemas/clubs/search.py (1)
6-11: Add field constraints and documentation.The model structure is good, but could benefit from additional validation and documentation:
class ClubSearchResult(TransfermarktBaseModel, IDMixin): - url: str - name: str - country: str - squad: int - market_value: Optional[int] = None + url: str = Field(..., description="Club's Transfermarkt URL", pattern="^https?://.*transfermarkt\..*") + name: str = Field(..., min_length=1, max_length=100) + country: str = Field(..., min_length=2, max_length=50) + squad: int = Field(..., ge=0, le=100, description="Number of players in the squad") + market_value: Optional[int] = Field(None, ge=0, description="Club's market value in euros")app/schemas/players/injuries.py (1)
16-19: Add validations for pagination fields.The class structure is good, but consider adding validations for pagination fields to prevent invalid states.
Consider applying these enhancements:
class PlayerInjuries(TransfermarktBaseModel, IDMixin, AuditMixin): - pageNumber: int - lastPageNumber: int - injuries: list[Injury] + pageNumber: int = Field(..., description="Current page number", gt=0) + lastPageNumber: int = Field(..., description="Last available page number", gt=0) + injuries: list[Injury] = Field(..., description="List of player injuries") + + @validator('lastPageNumber') + def validate_last_page(cls, v, values): + if 'pageNumber' in values and v < values['pageNumber']: + raise ValueError('lastPageNumber must be greater than or equal to pageNumber') + return vapp/schemas/players/stats.py (2)
1-3: Consider adding field validations for better data integrity.While the basic schema structure is good, consider importing and using Pydantic's field validators for enhanced data validation, especially for fields like
minutes_played(should be positive) andcompetition_id(format validation).Example additions:
from pydantic import Field, validator
19-20: Add documentation and list validation.The class could benefit from documentation and validation for the stats list.
Consider applying these improvements:
class PlayerStats(TransfermarktBaseModel, AuditMixin, IDMixin): - stats: list[PlayerStat] + """Collection of player statistics across different competitions and seasons.""" + stats: list[PlayerStat] = Field( + ..., + description="List of statistical records for the player", + min_items=1 + )app/schemas/competitions/search.py (1)
6-13: Consider enhancing schema with field descriptions and validations.While the basic structure is good, consider these improvements for better API documentation and data validation:
class CompetitionSearchResult(TransfermarktBaseModel, IDMixin): - name: str - country: str - clubs: int - players: int - total_market_value: Optional[int] = None - mean_market_value: Optional[int] = None - continent: Optional[str] = None + name: str = Field(..., description="Name of the competition") + country: str = Field(..., description="Country where the competition takes place") + clubs: int = Field(..., ge=0, description="Number of clubs in the competition") + players: int = Field(..., ge=0, description="Total number of players in the competition") + total_market_value: Optional[int] = Field(None, ge=0, description="Total market value in euros") + mean_market_value: Optional[int] = Field(None, ge=0, description="Average market value per club in euros") + continent: Optional[str] = Field(None, description="Continent where the competition takes place")Don't forget to add the import:
from pydantic import Fieldapp/schemas/players/search.py (1)
6-8: Consider adding validation for the club ID field.While the current implementation is functional, consider adding validation for the
idfield to ensure it matches your expected format (e.g., using regex or custom validators if club IDs follow a specific pattern).Example enhancement:
+from pydantic import constr class PlayerSearchClub(TransfermarktBaseModel): - id: str + id: constr(regex=r'^[0-9]+$') # Assuming numeric IDs name: strapp/schemas/players/transfers.py (3)
7-9: Consider adding field validation for id and name.While the basic structure is good, consider adding:
- Pattern validation for
idto ensure consistent format- Length constraints for
nameto prevent unreasonably long valuesclass PlayerTransferClub(TransfermarktBaseModel): - id: str - name: str + id: str = Field(..., pattern="^[A-Za-z0-9-]+$", description="Club identifier") + name: str = Field(..., min_length=1, max_length=100, description="Club name")
22-24: Consider adding constraints for youth_clubs list.The current implementation could benefit from:
- Length constraints for the list
- Validation for unique entries
- String constraints for club names
class PlayerTransfers(TransfermarktBaseModel, AuditMixin, IDMixin): transfers: list[PlayerTransfer] - youth_clubs: list[str] + youth_clubs: list[str] = Field( + default_list=[], + max_items=20, + unique_items=True, + description="List of youth clubs the player has been part of" + )
1-24: Consider adding class and field documentation.While the schema structure is solid, adding docstrings would help API consumers understand:
- The purpose of each class
- Field descriptions and constraints
- Examples of valid data
Example documentation style:
class PlayerTransfer(TransfermarktBaseModel, IDMixin): """ Represents a player transfer between two clubs. Examples: ```python transfer = PlayerTransfer( club_from={"id": "123", "name": "Club A"}, club_to={"id": "456", "name": "Club B"}, date=date(2023, 1, 1), upcoming=False, season="2023/24", market_value=1000000, fee="1000000" ) ``` """app/schemas/players/achievements.py (3)
6-9: Consider adding field descriptions for better documentation.The model structure is clean and appropriate. Consider adding field descriptions using Pydantic's Field class for better API documentation.
class AchievementDetail(TransfermarktBaseModel): - id: Optional[str] = None - name: str + id: Optional[str] = Field( + None, + description="Unique identifier for the achievement detail" + ) + name: str = Field( + ..., + description="Name of the achievement detail" + )
17-21: Add validation for the count field.The model structure is good, but consider adding validation to ensure the count is positive.
class PlayerAchievement(TransfermarktBaseModel): title: str - count: int + count: int = Field(..., gt=0, description="Number of times this achievement was earned") details: list[AchievementDetails]
23-24: Consider adding validation for non-empty achievements list.The model structure and inheritance are well-designed. Consider adding a validator to ensure the achievements list is not empty.
class PlayerAchievements(TransfermarktBaseModel, IDMixin, AuditMixin): - achievements: list[PlayerAchievement] + achievements: list[PlayerAchievement] = Field( + ..., + min_items=1, + description="List of player achievements" + )app/schemas/clubs/players.py (2)
21-21: Reconsider empty string default for status field.Using an empty string as default for the status field could be misleading. Consider using
Noneas the default value to clearly indicate the absence of a status.- status: Optional[str] = "" + status: Optional[str] = None
24-25: Add validation constraints to players list.Consider adding constraints to the players list to ensure it's not empty and has a reasonable maximum size.
class ClubPlayers(TransfermarktBaseModel): - players: list[ClubPlayer] + players: list[ClubPlayer] = Field( + ..., + min_items=1, + max_items=100, # Adjust based on your requirements + description="List of players in the club" + )app/api/endpoints/competitions.py (1)
Line range hint
1-23: Consider adding docstrings to the endpoint functions.While the code is functionally correct, adding docstrings would improve API documentation by providing more context about the endpoints' purpose and expected behavior.
Apply this enhancement:
@router.get("/search/{competition_name}", response_model=schemas.CompetitionSearch) def search_competitions(competition_name: str, page_number: Optional[int] = 1): + """ + Search for competitions by name. + + Args: + competition_name: Name of the competition to search for + page_number: Optional page number for paginated results + + Returns: + CompetitionSearch: Search results containing matching competitions + """ tfmkt = TransfermarktCompetitionSearch(query=competition_name, page_number=page_number) competitions = tfmkt.search_competitions() return competitions @router.get("/{competition_id}/clubs", response_model=schemas.CompetitionClubs) def get_competition_clubs(competition_id: str, season_id: Optional[str] = None): + """ + Get all clubs participating in a specific competition. + + Args: + competition_id: ID of the competition + season_id: Optional season ID to filter clubs + + Returns: + CompetitionClubs: List of clubs in the competition + """ tfmkt = TransfermarktCompetitionClubs(competition_id=competition_id, season_id=season_id) competition_clubs = tfmkt.get_competition_clubs() return competition_clubsapp/api/endpoints/clubs.py (1)
Line range hint
13-31: Consider adding docstrings to document response model behaviorThe endpoint configurations are well-structured, but adding docstrings would help explain the difference between
exclude_noneandexclude_defaultschoices for each endpoint.Example docstring format:
@router.get("/search/{club_name}", response_model=schemas.ClubSearch, response_model_exclude_none=True) def search_clubs(club_name: str, page_number: Optional[int] = 1) -> dict: """Search for clubs by name. Uses exclude_none=True to omit null fields from search results for cleaner responses. """app/schemas/clubs/profile.py (2)
14-20: Enhance field validation and add documentationConsider adding field validation and documentation to improve data integrity.
Here's a suggested improvement:
class ClubLeague(TransfermarktBaseModel): + """Represents the league information of a club.""" - id: str - name: str - country_id: str - country_name: str - tier: str + id: str = Field(min_length=1, pattern="^[A-Za-z0-9-]+$", description="League identifier") + name: str = Field(min_length=1, description="Name of the league") + country_id: str = Field(min_length=1, pattern="^[A-Za-z0-9-]+$", description="Country identifier") + country_name: str = Field(min_length=1, description="Name of the country") + tier: str = Field(min_length=1, description="League tier/division")
22-47: Consider architectural improvements for maintainabilityTo improve maintainability and reusability, consider:
- Creating custom validators for business rules (e.g., founding_on date validation)
- Extracting common patterns into reusable components
Example implementation:
from datetime import date from typing import Optional from pydantic import Field, HttpUrl, validator class ContactInfo(TransfermarktBaseModel): """Reusable contact information component.""" tel: str = Field(pattern=r'^\+?[\d\s-]+$') fax: str = Field(pattern=r'^\+?[\d\s-]+$') website: HttpUrl class ClubProfile(TransfermarktBaseModel): # ... other fields ... contact: ContactInfo @validator('founded_on') def validate_founding_date(cls, v): if v > date.today(): raise ValueError("Founding date cannot be in the future") return vpyproject.toml (1)
26-28: Consider documenting linter configuration changesThe significant version updates for black (24.8.0) and ruff (0.6.1) might introduce new rules or breaking changes in code formatting.
Consider adding a
.changesor similar documentation file to track formatting tool updates and their impact on the codebase.app/schemas/players/profile.py (3)
9-12: Consider adding country code validation.Consider using ISO 3166-1 country code validation for the
countryfield to ensure standardized country representations.+from pydantic import constr + class PlayerPlaceOfBirth(TransfermarktBaseModel): city: str - country: str + country: constr(min_length=2, max_length=2) # ISO 3166-1 alpha-2 code
19-29: Consider grouping contract-related fields into a separate model.The contract-related fields could be organized into a dedicated sub-model for better maintainability and clarity.
+class PlayerContract(TransfermarktBaseModel): + joined: date + expires: Optional[date] + option: Optional[str] + class PlayerClub(TransfermarktBaseModel): id: Optional[str] name: str - joined: date - contract_expires: Optional[date] - contract_option: Optional[str] + contract: PlayerContract # Retired player last_club_id: Optional[str] last_club_name: Optional[str] most_games_for: Optional[str]
31-34: Use HttpUrl type for URL field.For consistency with other URL fields in the codebase, consider using the
HttpUrltype from Pydantic.class PlayerAgent(TransfermarktBaseModel): name: Optional[str] - url: Optional[str] + url: Optional[HttpUrl]app/services/competitions/clubs.py (1)
54-57: LGTM! Consider adding type hints and documentation.The changes align well with the Pydantic schema implementation. The response structure is clean and follows consistent naming conventions.
Consider these improvements:
- def get_competition_clubs(self) -> dict: + def get_competition_clubs(self) -> dict[str, Any]: """ Retrieve and parse the list of football clubs participating in a specific competition. Returns: - dict: A dictionary containing the competition's unique identifier, name, season identifier, list of clubs - participating in the competition, and the timestamp of when the data was last updated. + dict[str, Any]: A dictionary containing: + - id (str): Competition's unique identifier + - name (str): Competition name + - seasonId (str): Season identifier + - clubs (list[dict]): List of participating clubs with their IDs and names """app/services/players/stats.py (2)
Line range hint
1-13: Consider using Pydantic models for request/response validationGiven this PR's objective to implement Pydantic schemas, consider:
- Defining a Pydantic model for the stats response structure
- Using Pydantic for input validation of player_id
Here's a suggested implementation:
from pydantic import BaseModel, Field from typing import List class PlayerStat(BaseModel): competition_id: str club_id: str season_id: str competition_name: str # Add other stat fields based on the headers class PlayerStatsResponse(BaseModel): id: str stats: List[PlayerStat]
Line range hint
30-61: Consider improving error handling and readabilityThe parsing logic could benefit from:
- Explicit error handling for malformed data
- Breaking down complex list comprehensions
- Validation of parsed data
Consider refactoring like this:
def __parse_player_stats(self) -> list: try: rows = self.page.xpath(Players.Stats.ROWS) if not rows: return [] # Extract headers headers = to_camel_case( ["Competition id", "Club id", "Season id", "Competition name"] + self.get_list_by_xpath(Players.Stats.HEADERS) ) # Extract IDs competitions_ids = [ extract_from_url(url) for url in self.get_list_by_xpath(Players.Stats.COMPETITIONS_URLS) ] clubs_ids = [ extract_from_url(url) for url in self.get_list_by_xpath(Players.Stats.CLUBS_URLS) ] # Parse stats stats = [] for row in rows: row_stats = [] for text in row.xpath(Players.Stats.DATA): if text == "\xa0": continue row_stats.extend(text.split("\xa0/\xa0")) stats.append(row_stats[1:]) # Skip first element # Combine data data = [ [comp_id, club_id] + stat for comp_id, club_id, stat in zip(competitions_ids, clubs_ids, stats) ] return [zip_lists_into_dict(headers, stat) for stat in data] except Exception as e: logger.error(f"Error parsing player stats: {e}") raise ValueError("Failed to parse player statistics") from eapp/services/players/transfers.py (2)
73-73: Update method documentation to reflect response changesThe response handling has been simplified, but the method's docstring still mentions "timestamp of when the data was last updated" which is no longer included in the response.
""" Retrieve and parse the transfer history and youth clubs of the specified player from Transfermarkt. Returns: - dict: A dictionary containing the player's unique identifier, parsed transfer history, youth clubs, - and the timestamp of when the data was last updated. + dict: A dictionary containing the player's unique identifier, parsed transfer history, and youth clubs. """
Line range hint
9-73: Consider service layer validation strategyWith the introduction of Pydantic schemas, consider whether some validation logic should be moved from this service class to the schemas. This could help maintain a clearer separation of concerns where:
- Service layer focuses on data fetching and transformation
- Pydantic schemas handle data validation and serialization
This would make the code more maintainable and reduce duplication of validation logic.
app/api/endpoints/players.py (2)
32-36: Consider adding error handling for invalid market value dataMarket value data might be unavailable or in an unexpected format. Consider adding appropriate error handling and documentation for such cases.
@router.get("/{player_id}/market_value", response_model=schemas.PlayerMarketValue, response_model_exclude_none=True) def get_player_market_value(player_id: str): tfmkt = TransfermarktPlayerMarketValue(player_id=player_id) + try: player_market_value = tfmkt.get_player_market_value() + if not player_market_value: + raise ValueError("Market value data not available") return player_market_value + except Exception as e: + raise HTTPException(status_code=404, detail=str(e))
Line range hint
18-71: Consider generating OpenAPI documentationThe addition of response models significantly improves API documentation capabilities. Consider generating and publishing OpenAPI documentation to help API consumers understand the new schema structures.
You could use FastAPI's built-in support for generating OpenAPI documentation and consider hosting it for easy access by API consumers. This would make the schema changes more visible and useful for integration purposes.
app/services/players/market_value.py (1)
57-57: LGTM! Consider documenting the response structureThe explicit key listing improves code maintainability and schema validation. Consider adding a docstring example of the response structure.
def __parse_market_value_history(self) -> list: """ Parse the market value history of a football player from the retrieved data. Returns: list: A list of dictionaries, where each dictionary represents a data point in the player's market value history. Each dictionary contains keys 'date', 'age', - 'clubID', 'clubName', and 'value' with their respective values. + 'clubId', 'clubName', and 'marketValue' with their respective values. + + Example: + [ + { + "date": "2023-01-01", + "age": 25, + "clubId": "123", + "clubName": "Example FC", + "marketValue": "€10.00m" + } + ] """app/services/players/achievements.py (2)
50-54: Consider adding error handling for URL extractionThe extraction logic is clean and well-organized, but consider adding error handling for cases where URLs might be malformed.
- competition_id = extract_from_url(competition_url) + try: + competition_id = extract_from_url(competition_url) + except ValueError: + competition_id = None
58-58: Clarify season ID extraction fallback logicThe season ID extraction logic using fallback between club_url and competition_url could be more explicit.
- "id": extract_from_url(club_url, "season_id") or extract_from_url(competition_url, "season_id"), + "id": ( + extract_from_url(club_url, "season_id") + if extract_from_url(club_url, "season_id") + else extract_from_url(competition_url, "season_id") + ),app/utils/utils.py (1)
140-140: LGTM! Standardizing on camelCase convention.The removal of special handling for "Id" → "ID" aligns well with:
- The PR objective of implementing pydantic schemas
- Pydantic's default alias generator which uses standard camelCase (e.g., "userId" not "userID")
This change promotes consistency across the codebase by following standard camelCase conventions used by pydantic and modern APIs.
app/services/players/profile.py (1)
Line range hint
1-88: Consider separating scraping and transformation logic.The current implementation mixes web scraping with data transformation. Consider:
- Creating a dedicated scraper class that handles the raw data extraction
- Using the current class solely for data transformation and schema validation
- This would improve testability and maintainability
Example structure:
class PlayerProfileScraper: def scrape(self) -> dict: # Handle raw data extraction using XPath class PlayerProfileService: def get_profile(self) -> PlayerProfileSchema: # Use scraper and handle transformationapp/services/clubs/profile.py (1)
Line range hint
1-90: Consider adding type hints for better IDE supportWhile the code is well-structured and documented, adding type hints to the response dictionary and its nested structures would improve IDE support and make the code more maintainable.
Example for the response structure:
from typing import TypedDict, List, Optional class SquadInfo(TypedDict): size: Optional[str] averageAge: Optional[str] foreigners: Optional[str] nationalTeamPlayers: Optional[str] class LeagueInfo(TypedDict): id: Optional[str] name: Optional[str] countryId: Optional[str] countryName: Optional[str] tier: Optional[str] class ClubProfileResponse(TypedDict): id: str url: Optional[str] name: Optional[str] # ... other fields ... squad: SquadInfo league: LeagueInfo historicalCrests: List[str]app/services/clubs/players.py (1)
1-8: Consider converting to Pydantic BaseModelSince this PR's objective is to implement pydantic schemas, consider converting this dataclass to a Pydantic model. This would provide automatic validation, serialization, and better integration with FastAPI's type system.
Example conversion:
-from dataclasses import dataclass +from pydantic import BaseModel, Field -@dataclass -class TransfermarktClubPlayers(TransfermarktBase): +class TransfermarktClubPlayers(TransfermarktBase, BaseModel): club_id: str = Field(default=None, description="The unique identifier of the football club") season_id: str = Field(default=None, description="The unique identifier of the season") URL: str = Field( default="https://www.transfermarkt.com/-/kader/verein/{club_id}/saison_id/{season_id}/plus/1", description="The URL template for the club's players page on Transfermarkt" )app/schemas/base.py (3)
12-14: Consider using timezone-aware datetimes inAuditMixinUsing
datetime.now()without specifying a timezone can lead to ambiguities, especially with daylight saving times and across different server locales. It's advisable to use timezone-aware datetimes to ensure consistency.Apply this diff to use UTC time:
from datetime import datetime +from datetime import timezone class AuditMixin(BaseModel): - updated_at: datetime = Field(default_factory=datetime.now) + updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc))
35-38: Broaden exception handling inparse_str_to_dateCurrently, only
parser.ParserErroris caught. However,dateutil.parser.parsecan also raiseValueErrorfor invalid date strings. To ensure robustness, consider catching both exceptions.Apply this diff to handle additional exceptions:
def parse_str_to_date(cls, v: str): try: return parser.parse(v).date() if v else None - except parser.ParserError: + except (parser.ParserError, ValueError): return None
63-65: Remove redundant.replace(".", "")inparse_str_to_intThe
.replace(".", "")method is called twice consecutively, which is unnecessary. Removing the duplicate call simplifies the code without affecting functionality.Apply this diff to eliminate the redundant method:
.replace("€", "") .replace(".", "") .replace("+", "") -.replace(".", "") .replace("'", "")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
app/api/endpoints/clubs.py(1 hunks)app/api/endpoints/competitions.py(1 hunks)app/api/endpoints/players.py(2 hunks)app/schemas/base.py(1 hunks)app/schemas/clubs/__init__.py(1 hunks)app/schemas/clubs/players.py(1 hunks)app/schemas/clubs/profile.py(1 hunks)app/schemas/clubs/search.py(1 hunks)app/schemas/competitions/__init__.py(1 hunks)app/schemas/competitions/clubs.py(1 hunks)app/schemas/competitions/search.py(1 hunks)app/schemas/players/__init__.py(1 hunks)app/schemas/players/achievements.py(1 hunks)app/schemas/players/injuries.py(1 hunks)app/schemas/players/jersey_numbers.py(1 hunks)app/schemas/players/market_value.py(1 hunks)app/schemas/players/profile.py(1 hunks)app/schemas/players/search.py(1 hunks)app/schemas/players/stats.py(1 hunks)app/schemas/players/transfers.py(1 hunks)app/services/clubs/players.py(3 hunks)app/services/clubs/profile.py(2 hunks)app/services/clubs/search.py(0 hunks)app/services/competitions/clubs.py(1 hunks)app/services/competitions/search.py(0 hunks)app/services/players/achievements.py(3 hunks)app/services/players/injuries.py(3 hunks)app/services/players/jersey_numbers.py(2 hunks)app/services/players/market_value.py(3 hunks)app/services/players/profile.py(4 hunks)app/services/players/search.py(0 hunks)app/services/players/stats.py(2 hunks)app/services/players/transfers.py(3 hunks)app/utils/utils.py(1 hunks)pyproject.toml(2 hunks)requirements.txt(1 hunks)
💤 Files with no reviewable changes (3)
- app/services/clubs/search.py
- app/services/competitions/search.py
- app/services/players/search.py
✅ Files skipped from review due to trivial changes (3)
- app/schemas/clubs/init.py
- app/schemas/competitions/init.py
- app/schemas/players/init.py
🔇 Additional comments (59)
app/schemas/players/jersey_numbers.py (2)
1-3: LGTM! Clean imports and proper structure.
The imports are specific and well-organized, following Python best practices.
1-11: Verify schema integration with the API endpoint.
The schema is referenced in the players endpoint. Let's verify its implementation.
app/schemas/competitions/clubs.py (2)
1-3: LGTM! Clean imports and proper structure.
The imports are specific and well-organized, bringing in the necessary base classes and mixins.
1-12: Verify schema usage in FastAPI endpoints.
Let's verify that these schemas are properly integrated with FastAPI endpoints and that the response models are correctly specified.
✅ Verification successful
Schema integration verified and properly implemented
The verification shows that:
- The schemas are properly defined with correct inheritance from
TransfermarktBaseModel,IDMixin, andAuditMixin - The service layer (
TransfermarktCompetitionClubs) correctly implements the data structure matching the schema - Test coverage exists with proper schema validation
- The endpoint is configured with
CompetitionClubsas the response model - The base models provide proper field validation and camelCase transformation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check FastAPI endpoint implementations using these schemas
# Look for endpoint definitions using CompetitionClubs schema
echo "Checking endpoints using CompetitionClubs schema:"
rg -l "CompetitionClubs" --type py
# Look for response_model usage with these schemas
echo "\nChecking response_model usage:"
ast-grep --pattern 'router.$_($$$, response_model=$_)'
Length of output: 699
Script:
#!/bin/bash
# Let's examine the endpoint implementation and test coverage
# Check the endpoint implementation
echo "Endpoint implementation in competitions.py:"
rg -A 10 "@router.get\(\"/{competition_id}/clubs\"" app/api/endpoints/competitions.py
# Check test coverage
echo "\nTest implementation:"
cat tests/competitions/test_competitions_clubs.py
# Verify service layer implementation
echo "\nService layer implementation:"
cat app/services/competitions/clubs.py
Length of output: 4297
Script:
#!/bin/bash
# Let's check the endpoint implementation with proper escaping
echo "Endpoint implementation:"
rg -A 10 '@router\.get\("/{competition_id}/clubs"' app/api/endpoints/competitions.py
# Check the base model and mixins
echo "\nBase model implementation:"
cat app/schemas/base.py
Length of output: 2595
Script:
#!/bin/bash
# One final check for the endpoint implementation with single quotes
echo "Endpoint implementation:"
rg -A 10 "@router.get('/{competition_id}/clubs'" app/api/endpoints/competitions.py
# Check schema imports
echo "\nSchema imports in competitions endpoint:"
rg "^from.*import.*CompetitionClubs" app/api/endpoints/competitions.py
Length of output: 526
app/schemas/players/market_value.py (2)
1-6: LGTM! Clean import structure
The imports are well-organized, separating standard library imports from application imports with appropriate spacing.
15-16: Verify the impact of renaming marketValueHistory
Before implementing the snake_case naming change, verify its usage in API responses and frontend code to ensure backward compatibility.
app/schemas/clubs/search.py (2)
1-5: LGTM! Clean imports and structure.
The imports are well-organized and appropriate for the schema implementation.
14-18: Add pagination constraints and field validation.
The search model could benefit from additional validation:
class ClubSearch(TransfermarktBaseModel, AuditMixin):
- query: str
- page_number: int
- last_page_number: int
- results: list[ClubSearchResult]
+ query: str = Field(..., min_length=1, max_length=100)
+ page_number: int = Field(..., ge=1)
+ last_page_number: int = Field(..., ge=1)
+ results: list[ClubSearchResult] = Field(..., max_items=100)Let's verify the AuditMixin implementation:
✅ Verification successful
Let me gather more information about the AuditMixin implementation and its usage pattern.
Add pagination constraints and field validation, AuditMixin usage is correct
The suggested field validations would improve data quality. The AuditMixin is correctly used in the ClubSearch model, consistent with other search models in the codebase. It adds an updated_at timestamp field, which is the expected behavior.
class ClubSearch(TransfermarktBaseModel, AuditMixin):
- query: str
- page_number: int
- last_page_number: int
- results: list[ClubSearchResult]
+ query: str = Field(..., min_length=1, max_length=100)
+ page_number: int = Field(..., ge=1)
+ last_page_number: int = Field(..., ge=1)
+ results: list[ClubSearchResult] = Field(..., max_items=100)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the AuditMixin implementation to ensure consistent usage
ast-grep --pattern 'class AuditMixin {
$$$
}'
# Check other schemas using AuditMixin for consistency
rg -l "AuditMixin" "app/schemas"
Length of output: 462
Script:
#!/bin/bash
# Check the AuditMixin implementation in base.py
cat app/schemas/base.py
# Check a few examples of AuditMixin usage in other schemas
cat app/schemas/players/search.py
cat app/schemas/competitions/search.py
Length of output: 3375
app/schemas/players/injuries.py (2)
1-4: LGTM! Clean imports and good code organization.
The imports are appropriate for the schema implementation, and the code structure follows Python conventions.
1-19: Verify schema integration with FastAPI endpoints.
The schema looks well-structured, but let's verify its integration with the endpoints.
✅ Verification successful
Let me check the actual endpoint implementation to verify the schema integration.
Schema is properly integrated with FastAPI endpoints
The schema is correctly integrated in the FastAPI endpoint at app/api/endpoints/players.py. The endpoint /{player_id}/injuries uses PlayerInjuries as the response model with proper configuration:
- Uses correct response_model annotation
- Includes response_model_exclude_none parameter
- Properly handles pagination through the page_number parameter
- Integrates with the corresponding service class TransfermarktPlayerInjuries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the schema is properly used in FastAPI endpoints
# Expected: Find endpoints using PlayerInjuries as response_model
# Search for endpoints using this schema
echo "Checking endpoints using PlayerInjuries schema..."
rg -l "PlayerInjuries" "app/api"
# Search for response_model usage
echo "Checking response_model usage..."
ast-grep --pattern 'response_model = PlayerInjuries'
Length of output: 298
Script:
#!/bin/bash
# Get the endpoint implementation details
echo "Checking endpoint implementation in players.py..."
rg -A 5 -B 5 "PlayerInjuries" "app/api/endpoints/players.py"
Length of output: 1345
app/schemas/competitions/search.py (2)
1-4: LGTM! Clean and well-structured imports.
The imports are appropriate and follow PEP 8 guidelines.
16-20: 🛠️ Refactor suggestion
Enhance search schema with validations and verify audit fields.
The pagination structure looks good, but consider adding validations:
class CompetitionSearch(TransfermarktBaseModel, AuditMixin):
- query: str
- page_number: int
- last_page_number: int
- results: list[CompetitionSearchResult]
+ query: str = Field(..., min_length=1, max_length=100, description="Search query string")
+ page_number: int = Field(..., ge=1, description="Current page number")
+ last_page_number: int = Field(..., ge=1, description="Total number of available pages")
+ results: list[CompetitionSearchResult] = Field(..., description="List of competition search results")Let's verify the AuditMixin implementation:
✅ Verification successful
Add field validations but AuditMixin implementation is correct
The AuditMixin is properly implemented with an updated_at field and is consistently used across all search schemas (competitions, players, clubs). The suggested field validations would improve the schema, but the audit functionality is correctly integrated.
app/schemas/base.py: AuditMixin properly definesupdated_atwith datetime default- Search schemas consistently inherit AuditMixin:
app/schemas/competitions/search.pyapp/schemas/players/search.pyapp/schemas/clubs/search.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AuditMixin implementation and usage
# Expected: Find AuditMixin definition and its audit fields
# Check AuditMixin definition
echo "Checking AuditMixin definition:"
rg -A 5 "class AuditMixin"
# Check if other search schemas use AuditMixin consistently
echo "Checking other search schemas:"
fd -e py -p ".*search\.py$" | xargs rg "AuditMixin"
Length of output: 1102
app/schemas/players/search.py (2)
1-3: LGTM! Clean and well-structured imports.
The imports are appropriate and follow good practices, with proper use of typing hints and base classes.
1-25: Verify schema consistency across the codebase.
Let's ensure this schema implementation is consistent with other similar schemas in the codebase.
✅ Verification successful
Let me gather more information about the schema implementations to ensure complete verification.
Schema implementation is consistent with similar patterns in the codebase
The implementation follows the established patterns seen in other search schemas:
- All search schemas consistently inherit from
TransfermarktBaseModel - Main search classes (
PlayerSearch,ClubSearch,CompetitionSearch) consistently useAuditMixin - Common pagination fields:
query,page_number/pageNumber,last_page_number/lastPageNumber - Result classes follow similar structure with domain-specific fields
- No circular imports detected, only standard exports in
__init__.pyfiles
Minor style inconsistency: page_number uses snake_case in other schemas while pageNumber uses camelCase in PlayerSearch.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar schema patterns and potential circular imports
echo "Checking for similar schema patterns..."
rg -g "*.py" "class.*Search.*BaseModel" app/schemas
echo "Checking for potential circular imports..."
rg -g "*.py" "from app.schemas.players.search import" app/
Length of output: 1004
Script:
#!/bin/bash
# Check the implementation details of similar search schemas
echo "Competition Search Schema:"
cat app/schemas/competitions/search.py
echo -e "\nClub Search Schema:"
cat app/schemas/clubs/search.py
# Check for any other potential imports of these search schemas
echo -e "\nChecking cross-references between search schemas:"
rg -g "*.py" "from app.schemas.(competitions|clubs|players).search import" app/
Length of output: 1638
app/schemas/players/transfers.py (1)
1-6: LGTM! Well-organized imports.
The imports are properly organized, following Python's import style guide with standard library imports first, followed by local imports.
app/schemas/players/achievements.py (2)
1-4: LGTM! Clean and focused imports.
The imports are minimal and purposeful, bringing in only the required dependencies.
11-15: Verify the consistency of required/optional fields.
The model structure looks good. The required season field ensures temporal context while keeping competition and club optional provides flexibility.
✅ Verification successful
Let me gather more information about the usage of this model by searching for references to it in the codebase.
Let me try again with corrected commands:
Let me check how this model is actually used in the related files:
Required/optional fields are correctly implemented and used consistently
The verification shows that the AchievementDetails model is correctly implemented and used consistently throughout the codebase:
- The model is used in the
PlayerAchievementschema which is part of the mainPlayerAchievementsresponse model - The service layer (
TransfermarktPlayerAchievements) correctly constructs the achievement details by:- Always providing the required
seasonfield with both id and name - Only including
clubandcompetitionfields when data is available, matching their optional nature
- Always providing the required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of AchievementDetails usage across the codebase
# Check for any hardcoded achievement details to ensure they match the schema
rg -A 5 "AchievementDetails\(" app/
# Check if there are any direct database queries or models that should align with this schema
ast-grep --pattern 'class $_(Achievement|.*Achievement.*)(Model)? {
$$$
}'
Length of output: 511
Script:
#!/bin/bash
# Search for any references to AchievementDetails in Python files
rg -t python "AchievementDetails" app/
# Search for any test files related to achievements
fd "test.*achievement.*\.py" app/
# Look for any serialization/deserialization of achievement data
rg -t python "achievement.*json|json.*achievement" app/
Length of output: 194
Script:
#!/bin/bash
# Search for any references to AchievementDetails
rg "AchievementDetails" app/
# Search for any test files related to achievements
find app/ -type f -name "test*achievement*.py"
# Look for any serialization/deserialization of achievement data
rg "achievement.*json|json.*achievement" app/
# Look for any files that might use this model
rg "achievement" -l app/
Length of output: 444
Script:
#!/bin/bash
# Check the content of files that might use AchievementDetails
cat app/schemas/players/achievements.py
cat app/services/players/achievements.py
cat app/api/endpoints/players.py
# Look for any validation or parsing of achievement data
rg -A 5 "achievement.*parse|parse.*achievement" app/
rg -A 5 "achievement.*validate|validate.*achievement" app/
Length of output: 8844
app/schemas/clubs/players.py (2)
1-6: LGTM! Clean and appropriate imports.
The imports are well-organized and correctly chosen for the schema requirements.
7-25: Verify schema usage in API endpoints.
Let's ensure these schemas are being used correctly and there are no conflicts.
✅ Verification successful
Let's examine how these schemas are actually used in the endpoint and check if there are any conflicting schema definitions with similar fields.
Schema usage verified and found to be correctly implemented
The ClubPlayer and ClubPlayers schemas are properly used in the API endpoint /clubs/{club_id}/players as the response model, with appropriate service integration through TransfermarktClubPlayers. No conflicting schema definitions were found that could cause issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for schema usage in API endpoints
echo "Checking API endpoints using these schemas..."
rg -l "ClubPlayer|ClubPlayers" app/api/
# Check for potential conflicting schemas
echo "Checking for potential conflicting schemas..."
rg -l "class.*Player" app/schemas/
Length of output: 601
Script:
#!/bin/bash
# Check how ClubPlayer/ClubPlayers are used in the API endpoint
echo "Checking usage in clubs.py endpoint..."
rg -A 5 "ClubPlayer|ClubPlayers" app/api/endpoints/clubs.py
# Check for any conflicting Player schemas in other files
echo "Checking player schema definitions..."
ast-grep --pattern 'class $_ {
$$$
name: str
$$$
}' app/schemas/players/*.py app/schemas/clubs/players.py
Length of output: 1163
app/api/endpoints/competitions.py (3)
5-5: LGTM! Clean schema import.
The schema import follows good practices with clear aliasing.
19-23: LGTM! Well-structured endpoint with proper response model.
The addition of response_model=schemas.CompetitionClubs improves API documentation and ensures runtime validation of the response data.
Let's verify the schema implementation:
✅ Verification successful
Schema implementation correctly matches the endpoint's response model
The schema implementation in app/schemas/competitions/clubs.py properly defines the CompetitionClubs model with all necessary fields and inherits from the correct base classes:
- Has required fields: id, name, season_id, and clubs (list of CompetitionClub)
- Inherits from TransfermarktBaseModel for consistent serialization
- Includes IDMixin and AuditMixin for standard fields
- CompetitionClub submodel is also properly defined
The endpoint's use of response_model=schemas.CompetitionClubs is valid and will provide proper validation and documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CompetitionClubs schema implementation
# Expected: Find the schema class definition to ensure it exists and matches the response structure
# Search for the schema definition
ast-grep --pattern 'class CompetitionClubs($_):
$$$'
# Check if the schema is being used consistently across the codebase
rg -g '!*.pyc' -g '!*test*' "CompetitionClubs"
Length of output: 960
Script:
#!/bin/bash
# Let's check the actual schema implementation to verify its structure
cat app/schemas/competitions/clubs.py
# Also check the base models it inherits from
rg -g '!*.pyc' "class TransfermarktBaseModel|class IDMixin|class AuditMixin" -A 5
Length of output: 997
12-16: LGTM! Well-structured endpoint with proper response model.
The addition of response_model=schemas.CompetitionSearch improves API documentation and ensures runtime validation of the response data.
Let's verify the schema implementation:
✅ Verification successful
Schema implementation verified and properly aligned with the endpoint
The CompetitionSearch schema is well-implemented and matches the response structure from TransfermarktCompetitionSearch service:
- Schema defines all required fields: query, page_number, last_page_number, and results
- CompetitionSearchResult schema correctly models each result with proper typing
- Service implementation returns data that aligns with the schema structure
- Response model validation will work as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CompetitionSearch schema implementation
# Expected: Find the schema class definition to ensure it exists and matches the response structure
# Search for the schema definition
ast-grep --pattern 'class CompetitionSearch($_):
$$$'
# Check if the schema is being used consistently across the codebase
rg -g '!*.pyc' -g '!*test*' "CompetitionSearch"
Length of output: 960
Script:
#!/bin/bash
# Let's search for the CompetitionSearch schema and its usage
# First, let's find all schema-related files
fd "\.py$" app/schemas/competitions/
# Then search for CompetitionSearch specifically
rg -g '!*.pyc' "CompetitionSearch"
# Look for any base schema classes that might be related
rg -g '!*.pyc' "class.*Search.*\(" app/schemas/
# Check the TransfermarktCompetitionSearch implementation
rg -g '!*.pyc' "class TransfermarktCompetitionSearch"
Length of output: 2279
Script:
#!/bin/bash
# Let's check the CompetitionSearch schema implementation details
cat app/schemas/competitions/search.py
# Also check the TransfermarktCompetitionSearch service implementation
cat app/services/competitions/search.py
Length of output: 3917
app/api/endpoints/clubs.py (3)
5-5: LGTM! Clean import organization
The schema import with alias provides good namespace organization and aligns with the PR's objective of implementing Pydantic schemas.
20-24: Verify schema compatibility with the profile response
The response model configuration looks good. Using exclude_defaults is appropriate for profile data as it provides a cleaner response while maintaining important null values.
✅ Verification successful
Schema compatibility verified successfully
The ClubProfile schema exists and matches the response structure from TransfermarktClubProfile:
- All required fields from the service response are properly defined in the schema
- Field types are appropriate (e.g., strings for text fields, Optional for nullable fields)
- Nested objects (squad, league) are properly structured
- The schema includes proper validation constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ClubProfile schema exists and matches response structure
# Check if the schema file exists and contains required fields
ast-grep --pattern 'class ClubProfile' -l 'app/schemas/clubs/profile.py'
# Check the schema structure
rg -A 10 'class ClubProfile' 'app/schemas/clubs/profile.py'
# Verify fields used in TransfermarktClubProfile
rg -A 5 'def get_club_profile' 'app/services/clubs/profile.py'
Length of output: 905
Script:
#!/bin/bash
# Let's try a different approach to verify the schema and response structure
# Check if ClubProfile schema exists and its structure
fd "profile.py" --exec cat {}
# Check TransfermarktClubProfile implementation
fd "TransfermarktClubProfile" --exec rg -A 10 "class TransfermarktClubProfile" {}
# Look for actual response structure
rg -A 15 "def get_club_profile" --type py
Length of output: 24476
Line range hint 27-31: Verify schema compatibility with seasonal player data
The response model configuration looks good, but we should verify the schema handles both seasonal and current player data correctly.
✅ Verification successful
Schema validation is properly implemented for both current and seasonal data
Based on the verification results:
- The schema is validated through test cases covering both current (
season_id=None) and historical seasons - Test cases validate the schema structure for club IDs 418 (current), 131 (2014), and 210 (2017)
- The service implementation correctly handles both scenarios by updating the season_id when not provided
- The response model excludes defaults appropriately, which is correct for varying player data structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ClubPlayers schema handles all cases
# Check if the schema file exists and contains required fields
ast-grep --pattern 'class ClubPlayers' -l 'app/schemas/clubs/players.py'
# Check the schema structure
rg -A 10 'class ClubPlayers' 'app/schemas/clubs/players.py'
# Verify how seasonal data is handled in the service
rg -A 5 'season_id' 'app/services/clubs/players.py'
Length of output: 1585
Script:
#!/bin/bash
# Let's verify the schema structure and service implementation with correct paths
# Check schema definition
ast-grep --pattern 'class ClubPlayers'
ast-grep --pattern 'class ClubPlayer'
# Check service implementation
rg -A 10 'class TransfermarktClubPlayers'
rg -A 5 'def get_club_players'
# Check if there are any test files validating the schema
fd -g '*test*.py' -x rg -l 'ClubPlayers'
Length of output: 1746
Script:
#!/bin/bash
# Let's check the test file and service implementation for schema validation
# Check test implementation for schema validation
rg -A 10 'ClubPlayers' tests/clubs/test_clubs_players.py
# Check the actual service implementation of get_club_players
rg -A 15 'def get_club_players' app/services/clubs/players.py
# Check if there are any schema validations in the service
rg -B 2 -A 10 'return.*players' app/services/clubs/players.py
Length of output: 1550
app/schemas/clubs/profile.py (1)
1-6: LGTM! Clean import organization
The imports are well-organized and all are actively used in the code.
pyproject.toml (2)
15-19: Verify FastAPI and Pydantic version compatibility
The FastAPI version 0.115.5 and Pydantic 2.8.2 combination needs verification:
- FastAPI has specific Pydantic version requirements
- This is a significant version jump for FastAPI
✅ Verification successful
FastAPI and Pydantic versions are compatible
The codebase is already using Pydantic v2 patterns:
- Uses
ConfigDictfor model configuration - Uses
field_validatorinstead of v1'svalidator - No usage of deprecated
BaseSettingsfound
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check FastAPI's requirements.txt for Pydantic version constraints
# Get FastAPI's requirements for version 0.115.5
curl -s https://raw.githubusercontent.com/tiangolo/fastapi/0.115.5/requirements.txt | grep -i pydantic
# Check if any files still use old Pydantic v1 patterns
rg "from pydantic import BaseModel" -A 5
Length of output: 421
Script:
#!/bin/bash
# Let's check for Pydantic v2-specific patterns to confirm v2 compatibility
rg "model_config = ConfigDict" -A 2
# Check for any v1 specific validation patterns that might need updates
rg "validator\(" -A 2
# Look for potential Pydantic v1 to v2 migration issues in settings
rg "from pydantic import.*BaseSettings" -A 5
Length of output: 948
3-3: Verify the major version bump rationale
The version bump to 3.0.0 suggests breaking changes. This aligns with the significant refactoring to implement Pydantic schemas, but let's ensure all breaking changes are documented.
Also applies to: 8-8
✅ Verification successful
Major version bump (3.0.0) is justified
The version bump from 2.0.3 to 3.0.0 is appropriate given the evidence of significant breaking changes:
- Complete refactoring to implement Pydantic schemas across the codebase
- Multiple commits showing systematic schema implementation:
- Implementation of base schemas
- Updates to services and utils
- Integration with endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CHANGELOG.md or similar documentation
# Look for changelog files
fd -i changelog
# Check README for migration guide
grep -i "migration\|breaking" README.md
Length of output: 421
Script:
#!/bin/bash
# Let's check for version-related changes and potential breaking changes
# Check git history for version changes
git log -p --grep="version" -- pyproject.toml
# Look for migration or schema-related changes
rg -l "pydantic" --type py
# Check commit messages for breaking changes
git log --oneline | grep -i "break\|schema\|pydantic"
Length of output: 376
app/schemas/players/profile.py (2)
1-7: LGTM! Clean and well-organized imports.
The imports are properly organized with clear separation between standard library, third-party, and local imports.
1-57: Verify schema completeness against Transfermarkt API.
Please ensure that all available fields from the Transfermarkt API are covered in this schema.
✅ Verification successful
Let me check the API endpoint implementation to see how the data is being handled and mapped to our schema.
Let me check the test files for the player profile to verify the schema against actual test data.
Schema implementation matches the Transfermarkt data structure
The schema implementation in app/schemas/players/profile.py correctly reflects all fields available from the Transfermarkt API as evidenced by:
- Test cases covering different player scenarios (active, retired)
- Service implementation showing all fields being extracted
- Field names and types matching the actual data structure
The schema handles all possible variations including:
- Optional fields (agent URL, contract options)
- Retired player specific fields (retired_since, last_club details)
- All social media and personal information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any TODO comments or documentation that might indicate missing fields
rg -i "todo|fixme|xxx" "app/schemas/players/"
# Look for any API response handling code that might reveal additional fields
rg -l "response" "app/api/endpoints/"
Length of output: 171
Script:
#!/bin/bash
# Check the players endpoint implementation for response handling
cat app/api/endpoints/players.py
# Look for any test files that might contain sample responses
fd -e json -e py test.*player
Length of output: 3576
Script:
#!/bin/bash
# Check the player profile test file for sample data
cat tests/players/test_players_profile.py
# Also check the service implementation to see how the data is mapped
cat app/services/players/profile.py
Length of output: 12129
app/services/players/jersey_numbers.py (2)
4-4: LGTM! Well-organized imports
The imports are clean and all imported utilities are actively used in the code.
58-58: Verify schema compatibility and update documentation
The removal of updatedAt and clean_response suggests that data validation is now handled by Pydantic schemas. Please ensure:
- The raw response structure matches the expected
PlayerJerseyNumbersPydantic schema - Update the method's docstring to reflect that the response is now validated through Pydantic
Let's verify the schema compatibility:
✅ Verification successful
Schema compatibility verified - no issues found
The raw response structure matches the expected Pydantic schema perfectly:
- The service returns
{"id": player_id, "jerseyNumbers": [...]} - This aligns with
PlayerJerseyNumbersschema which hasid(from IDMixin) andjersey_numbersfields - The
jersey_numbersfield contains a list ofJerseyNumberobjects withseason,club, andjersey_numberfields - FastAPI endpoint correctly uses
response_model=schemas.PlayerJerseyNumbers - Timestamp handling is automatically managed by
AuditMixin
The removal of clean_response and manual timestamp handling was the right approach as validation and camelCase conversion are handled by the Pydantic schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the response structure matches the Pydantic schema
# Find and display the PlayerJerseyNumbers schema definition
echo "Looking for PlayerJerseyNumbers schema definition..."
rg -A 10 "class PlayerJerseyNumbers"
# Find endpoint usage to verify response_model
echo "Checking endpoint implementation..."
rg -A 5 "/players/.*jersey_numbers"
Length of output: 459
Script:
#!/bin/bash
# Let's gather more comprehensive information about the schema and service implementation
# Check the complete schema definitions including imports
echo "=== Schema Definitions ==="
cat app/schemas/players/jersey_numbers.py
# Check the service implementation
echo -e "\n=== Service Implementation ==="
cat app/services/players/jersey_numbers.py
# Check for any FastAPI route definitions
echo -e "\n=== Route Definitions ==="
rg -l "jersey_numbers" app/api/
Length of output: 3105
Script:
#!/bin/bash
# Let's check the API endpoint implementation and any response model usage
echo "=== API Endpoint Implementation ==="
cat app/api/endpoints/players.py
# Let's also check the base models to understand the inheritance
echo -e "\n=== Base Models ==="
cat app/schemas/base.py
Length of output: 5612
app/services/competitions/clubs.py (1)
54-57: Verify schema consistency with other endpoints
Let's ensure the response structure aligns with the Pydantic schema definition and maintains consistency with other endpoints.
✅ Verification successful
Let me gather more information about the schema definition and its usage.
Let me check one more aspect to verify the schema consistency completely.
Schema consistency verified successfully
The schema implementation is consistent:
- The Pydantic schema
CompetitionClubscorrectly definesseason_idas a required string field - The endpoint is properly decorated with the response model
- The
seasonIdextraction is unique to this endpoint and correctly maps to the schema'sseason_idfield - The schema is properly imported and used across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check schema definition and usage patterns
# Find the corresponding schema definition
echo "Looking for competition clubs schema definition..."
rg -p "class CompetitionClubs" app/
# Check for similar patterns in other endpoints
echo "Checking similar response patterns in other endpoints..."
ast-grep --pattern 'def get_$_($$$) -> dict:
$$$
response["seasonId"] = $_
$$$'
Length of output: 464
Script:
#!/bin/bash
# Check the schema definition details
echo "Checking CompetitionClubs schema fields..."
rg -A 10 "class CompetitionClubs" app/schemas/competitions/clubs.py
# Check other files using this schema
echo "Finding usages of CompetitionClubs schema..."
rg "CompetitionClubs" app/
# Check similar season_id extractions
echo "Checking similar season_id extractions..."
rg -p "seasonId.*extract_from_url" app/
Length of output: 1426
Script:
#!/bin/bash
# Check the response structure in the service
echo "Checking response structure in TransfermarktCompetitionClubs..."
rg -A 20 "def get_response" app/services/competitions/clubs.py
# Check extract_from_url implementation
echo "Checking extract_from_url implementation..."
rg -p "def extract_from_url" app/
Length of output: 442
requirements.txt (3)
1-35: Breaking changes warning: Major version updates detected
Several dependencies have major version updates that might introduce breaking changes:
- anyio: 3.x → 4.x
- fastapi: 0.104.x → 0.115.x
- starlette: 0.x → 0.41.x
- pydantic: 1.x → 2.x (implied from AI summary)
#!/bin/bash
# Description: Look for potential usage patterns affected by breaking changes
# Check FastAPI response_model usage (mentioned in PR summary)
rg "response_model"
# Check for FastAPI dependency usage patterns
rg "Depends\("
# Look for direct Starlette imports that might be affected
rg "from starlette\."Consider:
- Adding a changelog entry documenting these major version changes
- Running the full test suite to catch any breaking changes
- Updating the CI pipeline to include compatibility checks
10-10: Verify FastAPI ecosystem compatibility
The FastAPI ecosystem components have been updated to:
- fastapi==0.115.5
- pydantic==2.8.2
- pydantic-core==2.20.1
- starlette==0.41.2
These versions are compatible, but the upgrade to Pydantic v2 may require code changes.
#!/bin/bash
# Description: Check for potential breaking changes in Pydantic v2 migration
# Look for common Pydantic v1 patterns that need updates in v2
# Check for BaseSettings usage (replaced by Settings in v2)
rg -l "BaseSettings"
# Check for validator decorators (syntax changed in v2)
rg -l "@validator"
# Check for Config classes (renamed to model_config in v2)
rg -l "class Config:"
# Check for any direct field_name.dict() calls (replaced with model_dump in v2)
rg ".dict\(\)"Consider adding a migration guide in the documentation for any developers who need to update their code to work with Pydantic v2.
Also applies to: 18-20, 27-27
4-4: Verify security implications of major version updates
Notable version jumps in security-critical packages:
- certifi==2024.8.30
- requests==2.32.3
- urllib3==2.2.3
Also applies to: 23-23, 29-29
✅ Verification successful
Security review of package updates completed - all versions are secure
Based on the GitHub Security Advisory data:
- requests 2.32.3: Latest version, includes fixes for all known vulnerabilities (last patched version needed was 2.32.0)
- urllib3 2.2.3: Latest version, includes fixes for all known vulnerabilities (last patched version needed was 2.2.2)
Let me verify certifi as well before concluding:
All package versions are secure and up-to-date
The security analysis confirms all packages are using secure versions:
- certifi 2024.8.30: Latest version, includes all security patches (last needed was 2024.07.04)
- requests 2.32.3: Latest version, includes all security patches (last needed was 2.32.0)
- urllib3 2.2.3: Latest version, includes all security patches (last needed was 2.2.2)
All version updates include important security fixes and are recommended. No security concerns with these updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known security vulnerabilities in updated packages
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Repeat for urllib3 and certifi
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "urllib3") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 3148
Script:
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "certifi") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1024
app/services/players/stats.py (1)
Line range hint 22-28: LGTM! Good error handling and initialization.
The initialization logic is well-structured with proper error handling.
app/services/players/transfers.py (1)
4-4: LGTM! Clean import structure
The removal of unused imports and retention of necessary utilities improves code cleanliness.
app/api/endpoints/players.py (4)
5-5: LGTM! Clean and well-organized imports
The schema import is properly aliased and follows Python best practices.
25-29: LGTM! Clean endpoint implementation
The endpoint follows consistent patterns and properly implements response validation.
18-22: Verify schema compatibility with the service response
The endpoint correctly implements response validation, but let's verify that the PlayerSearch schema matches the structure returned by TransfermarktPlayerSearch.
#!/bin/bash
# Description: Verify schema compatibility with service response
# Search for the schema definition and service return structure
# Find PlayerSearch schema definition
echo "=== PlayerSearch Schema ==="
ast-grep --pattern 'class PlayerSearch$$$'
# Find service return structure
echo "=== Service Implementation ==="
ast-grep --pattern 'class TransfermarktPlayerSearch$$$return$$$'Line range hint 46-71: Verify all schema implementations exist
All endpoints are implemented consistently with proper response models. Let's verify that all referenced schemas are properly implemented.
✅ Verification successful
Let me search for the schemas in a different way since they might be in a specific schemas module.
Let me try one more search with ripgrep to find these schema definitions anywhere in the codebase.
All schema implementations are properly defined
All required schema classes are correctly implemented in their respective files under the app/schemas/players directory:
PlayerJerseyNumbersin jersey_numbers.pyPlayerStatsin stats.pyPlayerInjuriesin injuries.pyPlayerAchievementsin achievements.py
Each schema properly inherits from TransfermarktBaseModel and includes appropriate mixins (AuditMixin, IDMixin) with their respective type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all schema implementations exist
echo "=== Checking for all required schemas ==="
for schema in PlayerJerseyNumbers PlayerStats PlayerInjuries PlayerAchievements; do
echo "Looking for $schema..."
ast-grep --pattern "class $schema$$$"
done
Length of output: 247
Script:
#!/bin/bash
# Search for schema files and their content
echo "=== Looking for schema files ==="
fd "schemas?.py"
echo -e "\n=== Checking schema content ==="
for file in $(fd "schemas?.py"); do
echo -e "\n=== Content of $file ==="
cat "$file"
done
Length of output: 192
Script:
#!/bin/bash
# Search for schema class definitions using ripgrep
echo "=== Searching for schema classes ==="
rg "class Player(Jersey|Stats|Injuries|Achievements)" -A 5
echo -e "\n=== Searching in app directory ==="
fd . app -t f -x cat {} | rg "class Player(Jersey|Stats|Injuries|Achievements)" -A 5
Length of output: 1993
app/services/players/market_value.py (3)
6-6: LGTM! Import changes align with code requirements
The imports are correctly maintained after removing the unused datetime module.
49-54: Verify consistent key naming across the codebase
The renaming of keys to camelCase (value → marketValue, clubID → clubId) aligns with schema standardization. Let's verify this naming convention is consistent across related files.
✅ Verification successful
The initial search wasn't specific enough. Let's search for the actual key usage patterns in the services layer to verify the consistency of the naming convention.
Key naming convention is consistently implemented
The verification shows that the camelCase naming convention is consistently followed across the codebase:
- Response objects use camelCase (
marketValue,clubId,marketValueHistory) - Internal processing and schema definitions use snake_case (
market_value,club_id) - The transformation from snake_case to camelCase happens at the service layer before returning responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent key naming in related files
# Look for variations of these keys to ensure consistent naming
# Search for potential inconsistencies in key naming
rg -g '*.py' "['\"](value|club[Ii][Dd])['\"]" app/
Length of output: 817
Script:
#!/bin/bash
# Let's check the actual usage of these keys in the codebase
rg -g '*.py' "(market_?[Vv]alue|club_?[Ii][Dd])" app/services/players/
Length of output: 2418
77-77: Verify Pydantic schema implementation
The removal of response cleaning suggests validation is handled by Pydantic schemas. Let's verify the schema implementation.
✅ Verification successful
Let me gather more context about the schema implementation and validation.
Let me check one more aspect to verify the schema usage in the market value service.
Based on the gathered evidence, I can now provide the final response:
Proper schema validation is implemented
The removal of response cleaning is justified as the validation is properly handled by Pydantic schemas:
PlayerMarketValueschema inherits fromTransfermarktBaseModeland includes proper typing- The API endpoint correctly uses
response_model=schemas.PlayerMarketValue - Response structure in the service matches the defined schema fields
TransfermarktBaseModelprovides base validation and camelCase transformation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper Pydantic schema implementation for market value responses
# Look for related schema definitions
rg -g '*.py' -A 5 "class.*MarketValue.*BaseModel" app/
# Check for response_model usage with this endpoint
rg -g '*.py' "market.?value.*response_model" app/
Length of output: 817
Script:
#!/bin/bash
# Check TransfermarktBaseModel definition and validation
rg -g '*.py' -A 5 "class TransfermarktBaseModel" app/
# Check market value service usage of schemas
rg -g '*.py' -A 10 "class MarketValue" app/services/
Length of output: 396
Script:
#!/bin/bash
# Check market value service implementation
rg -g '*.py' -A 15 "def get_market_value" app/services/
# Check the actual response construction
rg -g '*.py' -B 5 -A 5 "self\.response" app/services/players/market_value.py
Length of output: 837
app/services/players/injuries.py (1)
84-84: Verify the impact of removing updatedAt field
The removal of the updatedAt field is a breaking change that could affect API consumers. Let's verify if this change is properly documented and if there are any direct consumers of this field.
✅ Verification successful
Breaking change appears to be part of a broader API update
The removal of updatedAt field is part of a systematic change across multiple services, as evidenced by:
- The field has been removed from the injuries response schema
- Test files across multiple domains (players, clubs, competitions) show consistent updates removing
updatedAtassertions - The change appears to be intentional as it affects the entire API surface, not just the injuries endpoint
The removal seems to be a deliberate API simplification rather than an accidental change. Since this appears to be part of a coordinated API update and the field was primarily used in tests rather than business logic, this change is likely safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of updatedAt field from injuries response
rg -l "injuries.*updatedAt|updatedAt.*injuries" --type py
# Check if API changes are documented
fd -g "CHANGELOG.md" -g "README.md" -g "docs/*.md" -X grep -l "updatedAt"
Length of output: 282
Script:
#!/bin/bash
# Search for any direct usage of updatedAt field in Python files
rg -A 3 "updatedAt" --type py
# Check if documentation files exist and contain updatedAt references
fd -e md -X grep -l "updatedAt" || true
# Look for API response structures or tests involving injuries
rg -A 5 "class.*Injur|def.*injur" --type py
Length of output: 7944
app/services/players/achievements.py (3)
4-4: LGTM: Appropriate utility imports
The imported utilities are used effectively throughout the code for URL parsing and string cleaning.
56-61: LGTM: Well-structured achievement details
The nested dictionary structure is clean and aligns well with the Pydantic schema implementation objective.
98-98: Verify response validation with Pydantic schema
The response handling has been simplified, which is good. Let's verify that the response structure is properly validated by the Pydantic schema.
✅ Verification successful
Response validation is properly handled by Pydantic schemas
The response structure is well-validated through comprehensive Pydantic schemas:
PlayerAchievementsschema inherits fromTransfermarktBaseModel,IDMixin, andAuditMixin- Response structure is strictly typed with nested models for achievements, details, and metadata
- The service's response matches the schema structure with
idandachievementsfields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Pydantic schema implementation for player achievements
# Look for achievement schema definition
echo "Searching for achievement schema definition..."
rg -l "class.*Achievement.*BaseModel" --type py
# Check for response_model usage in routes
echo "Checking for response_model usage in routes..."
rg "achievements.*response_model" --type py
Length of output: 484
Script:
#!/bin/bash
# Let's check the schema implementation and response structure
# Check achievement schema content
echo "Achievement schema content:"
cat app/schemas/players/achievements.py
# Check the endpoint implementation
echo -e "\nEndpoint implementation:"
rg -A 5 -B 5 "@router.get\(\"/{player_id}/achievements\"" app/api/endpoints/players.py
Length of output: 1006
Script:
#!/bin/bash
# Let's check the endpoint implementation with fixed regex
echo "Endpoint implementation:"
rg -A 5 -B 5 '@router\.get\("/{player_id}/achievements"' app/api/endpoints/players.py
# Also check the achievements service implementation
echo -e "\nAchievements service implementation:"
cat app/services/players/achievements.py
Length of output: 4105
Script:
#!/bin/bash
# Let's check the endpoint implementation with a different approach
echo "Endpoint implementation:"
rg -A 5 -B 5 "player_id/achievements" app/api/endpoints/players.py
# Check base models to verify AuditMixin
echo -e "\nBase models:"
cat app/schemas/base.py
Length of output: 2410
app/utils/utils.py (1)
135-135: LGTM! Clean docstring formatting.
The docstring formatting maintains consistency with the rest of the codebase.
app/services/players/profile.py (3)
5-5: LGTM! Clean import changes.
The removal of unused imports and retention of necessary ones improves code cleanliness.
88-88: Verify response validation with Pydantic schemas.
The removal of clean_response suggests validation is now handled by Pydantic schemas. Let's verify the schema implementation.
#!/bin/bash
# Description: Verify that proper Pydantic validation is in place
# Search for the player profile schema
rg -t python "class.*PlayerProfile.*Schema.*BaseModel" app/schemas/
# Search for FastAPI endpoints using this service to ensure response_model is set
ast-grep --pattern 'app.get($path)
def $func():
$profile = TransfermarktPlayerProfile($args)
$$$'45-45: Verify schema consistency for renamed fields.
The field naming changes to camelCase (imageUrl, lastClubId) look good, but let's ensure they match the Pydantic schema definitions.
Also applies to: 76-76
✅ Verification successful
Field names are consistent with schema definitions
Looking at app/schemas/players/profile.py, I can confirm:
image_urlis defined asimage_url: HttpUrlin thePlayerProfileschemalast_club_idis defined aslast_club_id: Optional[str]in thePlayerClubschema
The camelCase field names in the response are correct because the base schema TransfermarktBaseModel uses alias_generator=to_camel in its config, which automatically converts snake_case schema fields to camelCase in the JSON response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the field names in the Pydantic schemas match the response fields
# Search for Pydantic schema definitions related to player profiles
rg -t python "class.*Schema.*BaseModel" app/schemas/
# Search for specific field definitions
rg -t python "image.*:|last.*club.*:" app/schemas/
Length of output: 205
Script:
#!/bin/bash
# Let's try different approaches to find schema definitions
# Search for any Python files in schemas directory
fd -e py . app/schemas/
# Search for BaseModel imports and class definitions
rg "from pydantic import.*BaseModel" -A 5
rg "class.*\(.*BaseModel.*\):.*" -A 10
# Search for field definitions in Python files
rg "image.*=.*Field|last.*club.*=.*Field"
Length of output: 13917
app/services/clubs/profile.py (2)
5-5: LGTM: Clean import organization
The explicit import of utility functions is well-organized and follows good practices.
82-82: LGTM: Consistent naming convention
The rename from countryID to countryId maintains consistency with camelCase naming convention used throughout the response structure.
app/services/clubs/players.py (2)
133-133: Verify API compatibility after removing updatedAt field
The removal of the updatedAt field might affect API consumers. Please ensure this change is:
- Documented in the API changelog
- Reflected in the new pydantic schemas
- Communicated to API consumers if this is a breaking change
83-83: Verify the source of None values in page_players_infos
While filtering out None values is a good defensive practice, it's important to understand why they might occur. Could you verify if these None values are expected or if they indicate potential issues with the data scraping?
app/schemas/base.py (2)
8-10: LGTM: IDMixin class is correctly defined
The IDMixin class is properly defined with an id field of type str, ensuring consistent identification across models.
16-17: LGTM: TransfermarktBaseModel configured correctly
The TransfermarktBaseModel is set up with the alias_generator to convert field names to camelCase, which aligns with the expected JSON formatting conventions.
| class JerseyNumber(TransfermarktBaseModel): | ||
| season: str | ||
| club: str | ||
| jersey_number: int | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validation and documentation.
Consider adding:
- Field validation for proper data constraints
- Class documentation explaining the schema's purpose
Apply these improvements:
class JerseyNumber(TransfermarktBaseModel):
+ """Represents a player's jersey number for a specific season and club.
+
+ Attributes:
+ season: Season in format "YYYY/YY" (e.g., "2023/24")
+ club: Name of the club
+ jersey_number: Player's jersey number
+ """
- season: str
+ season: str = Field(
+ pattern=r"^\d{4}/\d{2}$",
+ description="Season in format YYYY/YY"
+ )
club: str
- jersey_number: int
+ jersey_number: int = Field(
+ gt=0,
+ le=99,
+ description="Jersey number between 1 and 99"
+ )📝 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.
| class JerseyNumber(TransfermarktBaseModel): | |
| season: str | |
| club: str | |
| jersey_number: int | |
| class JerseyNumber(TransfermarktBaseModel): | |
| """Represents a player's jersey number for a specific season and club. | |
| Attributes: | |
| season: Season in format "YYYY/YY" (e.g., "2023/24") | |
| club: Name of the club | |
| jersey_number: Player's jersey number | |
| """ | |
| season: str = Field( | |
| pattern=r"^\d{4}/\d{2}$", | |
| description="Season in format YYYY/YY" | |
| ) | |
| club: str | |
| jersey_number: int = Field( | |
| gt=0, | |
| le=99, | |
| description="Jersey number between 1 and 99" | |
| ) |
| class CompetitionClubs(TransfermarktBaseModel, IDMixin, AuditMixin): | ||
| id: str | ||
| name: str | ||
| season_id: str | ||
| clubs: list[CompetitionClub] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant ID field and enhance field validations.
- The
idfield appears redundant as it's already provided byIDMixin - Consider adding constraints for
season_idand other fields
class CompetitionClubs(TransfermarktBaseModel, IDMixin, AuditMixin):
+ """Represents a collection of clubs participating in a competition for a specific season."""
- id: str
- name: str
- season_id: str
- clubs: list[CompetitionClub]
+ name: str = Field(
+ min_length=1,
+ max_length=100,
+ description="The name of the competition"
+ )
+ season_id: str = Field(
+ regex="^[0-9]{4}$",
+ description="The season year identifier"
+ )
+ clubs: list[CompetitionClub] = Field(
+ min_items=1,
+ description="List of clubs participating in the competition"
+ )📝 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.
| class CompetitionClubs(TransfermarktBaseModel, IDMixin, AuditMixin): | |
| id: str | |
| name: str | |
| season_id: str | |
| clubs: list[CompetitionClub] | |
| class CompetitionClubs(TransfermarktBaseModel, IDMixin, AuditMixin): | |
| """Represents a collection of clubs participating in a competition for a specific season.""" | |
| name: str = Field( | |
| min_length=1, | |
| max_length=100, | |
| description="The name of the competition" | |
| ) | |
| season_id: str = Field( | |
| regex="^[0-9]{4}$", | |
| description="The season year identifier" | |
| ) | |
| clubs: list[CompetitionClub] = Field( | |
| min_items=1, | |
| description="List of clubs participating in the competition" | |
| ) |
| class PlayerMarketValue(TransfermarktBaseModel, IDMixin, AuditMixin): | ||
| marketValueHistory: list[MarketValueHistory] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert field name to snake_case
The field name marketValueHistory uses camelCase, which is inconsistent with Python's naming conventions (PEP 8). Consider renaming it to snake_case:
class PlayerMarketValue(TransfermarktBaseModel, IDMixin, AuditMixin):
- marketValueHistory: list[MarketValueHistory]
+ market_value_history: list[MarketValueHistory]📝 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.
| class PlayerMarketValue(TransfermarktBaseModel, IDMixin, AuditMixin): | |
| marketValueHistory: list[MarketValueHistory] | |
| class PlayerMarketValue(TransfermarktBaseModel, IDMixin, AuditMixin): | |
| market_value_history: list[MarketValueHistory] |
| class Injury(TransfermarktBaseModel): | ||
| season: str | ||
| injury: str | ||
| from_date: date | ||
| until_date: date | ||
| days: int | ||
| gamesMissed: int | ||
| gamesMissedClubs: list[str] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field descriptions and validations for data integrity.
While the schema structure is good, consider enhancing it with:
- Field descriptions using docstrings or Field(..., description="...")
- Date validation to ensure from_date precedes until_date
- Value constraints for numerical fields
Consider applying these enhancements:
class Injury(TransfermarktBaseModel):
- season: str
- injury: str
- from_date: date
- until_date: date
- days: int
- gamesMissed: int
- gamesMissedClubs: list[str]
+ season: str = Field(..., description="Season when the injury occurred", pattern=r"^\d{4}/\d{4}$")
+ injury: str = Field(..., description="Type or description of the injury")
+ from_date: date = Field(..., description="Date when the injury started")
+ until_date: date = Field(..., description="Date when the injury ended")
+ days: int = Field(..., description="Duration of injury in days", ge=0)
+ gamesMissed: int = Field(..., description="Number of games missed due to injury", ge=0)
+ gamesMissedClubs: list[str] = Field(..., description="List of clubs affected during injury period")
+
+ @validator('until_date')
+ def validate_dates(cls, v, values):
+ if 'from_date' in values and v < values['from_date']:
+ raise ValueError('until_date must be after from_date')
+ return v📝 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.
| class Injury(TransfermarktBaseModel): | |
| season: str | |
| injury: str | |
| from_date: date | |
| until_date: date | |
| days: int | |
| gamesMissed: int | |
| gamesMissedClubs: list[str] | |
| class Injury(TransfermarktBaseModel): | |
| season: str = Field(..., description="Season when the injury occurred", pattern=r"^\d{4}/\d{4}$") | |
| injury: str = Field(..., description="Type or description of the injury") | |
| from_date: date = Field(..., description="Date when the injury started") | |
| until_date: date = Field(..., description="Date when the injury ended") | |
| days: int = Field(..., description="Duration of injury in days", ge=0) | |
| gamesMissed: int = Field(..., description="Number of games missed due to injury", ge=0) | |
| gamesMissedClubs: list[str] = Field(..., description="List of clubs affected during injury period") | |
| @validator('until_date') | |
| def validate_dates(cls, v, values): | |
| if 'from_date' in values and v < values['from_date']: | |
| raise ValueError('until_date must be after from_date') | |
| return v |
| class PlayerStat(TransfermarktBaseModel): | ||
| competition_id: str | ||
| competition_name: str | ||
| season_id: str | ||
| club_id: str | ||
| appearances: int | ||
| goals: Optional[int] | ||
| assists: Optional[int] | ||
| yellow_cards: Optional[int] | ||
| red_cards: Optional[int] | ||
| minutes_played: Optional[int] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field constraints and documentation for better data integrity.
The schema could benefit from:
- Field constraints (e.g., non-negative integers for statistics)
- Cross-field validations (e.g., minutes_played should align with appearances)
- Documentation for the fields
Consider applying these improvements:
class PlayerStat(TransfermarktBaseModel):
+ """Represents statistical data for a player in a specific competition and season."""
competition_id: str
competition_name: str
season_id: str
club_id: str
- appearances: int
+ appearances: int = Field(ge=0, description="Number of appearances in the competition")
- goals: Optional[int]
+ goals: Optional[int] = Field(None, ge=0, description="Number of goals scored")
- assists: Optional[int]
+ assists: Optional[int] = Field(None, ge=0, description="Number of assists provided")
- yellow_cards: Optional[int]
+ yellow_cards: Optional[int] = Field(None, ge=0, description="Number of yellow cards received")
- red_cards: Optional[int]
+ red_cards: Optional[int] = Field(None, ge=0, description="Number of red cards received")
- minutes_played: Optional[int]
+ minutes_played: Optional[int] = Field(None, ge=0, description="Total minutes played")
+
+ @validator('minutes_played')
+ def validate_minutes_played(cls, v, values):
+ if v is not None and values.get('appearances', 0) > 0:
+ if v > values['appearances'] * 90 * 2: # allowing for extra time
+ raise ValueError("minutes_played seems unreasonably high for the number of appearances")
+ return v📝 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.
| class PlayerStat(TransfermarktBaseModel): | |
| competition_id: str | |
| competition_name: str | |
| season_id: str | |
| club_id: str | |
| appearances: int | |
| goals: Optional[int] | |
| assists: Optional[int] | |
| yellow_cards: Optional[int] | |
| red_cards: Optional[int] | |
| minutes_played: Optional[int] | |
| class PlayerStat(TransfermarktBaseModel): | |
| """Represents statistical data for a player in a specific competition and season.""" | |
| competition_id: str | |
| competition_name: str | |
| season_id: str | |
| club_id: str | |
| appearances: int = Field(ge=0, description="Number of appearances in the competition") | |
| goals: Optional[int] = Field(None, ge=0, description="Number of goals scored") | |
| assists: Optional[int] = Field(None, ge=0, description="Number of assists provided") | |
| yellow_cards: Optional[int] = Field(None, ge=0, description="Number of yellow cards received") | |
| red_cards: Optional[int] = Field(None, ge=0, description="Number of red cards received") | |
| minutes_played: Optional[int] = Field(None, ge=0, description="Total minutes played") | |
| @validator('minutes_played') | |
| def validate_minutes_played(cls, v, values): | |
| if v is not None and values.get('appearances', 0) > 0: | |
| if v > values['appearances'] * 90 * 2: # allowing for extra time | |
| raise ValueError("minutes_played seems unreasonably high for the number of appearances") | |
| return v |
| "clubFrom": { | ||
| "id": extract_from_url(transfer["from"]["href"]), | ||
| "name": transfer["from"]["clubName"], | ||
| }, | ||
| "to": { | ||
| "clubID": extract_from_url(transfer["to"]["href"]), | ||
| "clubName": transfer["to"]["clubName"], | ||
| "clubTo": { | ||
| "id": extract_from_url(transfer["to"]["href"]), | ||
| "name": transfer["to"]["clubName"], |
There was a problem hiding this comment.
💡 Codebase verification
Schema mismatch detected: Response uses camelCase while schema uses snake_case
The response structure in transfers.py uses clubFrom and clubTo, but the Pydantic schema in app/schemas/players/transfers.py defines these fields as club_from and club_to. While the nested structure with id and name matches correctly, the casing convention differs.
transfers.py: UsesclubFromandclubTo(camelCase)- Schema: Uses
club_fromandclub_to(snake_case)
🔗 Analysis chain
Verify alignment with Pydantic schema
The restructured club data format looks good and follows consistent naming conventions. Let's verify it matches the new Pydantic schema structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the response structure matches the Pydantic schema definition
# Look for corresponding Pydantic schema file
fd -e py -E "tests" . | xargs rg -l "class.*Transfer.*BaseModel"
# Search for schema structure
ast-grep --pattern 'class Transfer$_: BaseModel:
$$$
club_from: $_
club_to: $_
$$$'
Length of output: 598
Script:
#!/bin/bash
# Let's check the specific transfer schema in players/transfers.py
cat app/schemas/players/transfers.py
Length of output: 595
| @router.get("/{player_id}/transfers", response_model=schemas.PlayerTransfers, response_model_exclude_none=True) | ||
| def get_player_transfers(player_id: str): | ||
| tfmkt = TransfermarktPlayerTransfers(player_id=player_id) | ||
| player_market_value = tfmkt.get_player_transfers() | ||
| return player_market_value |
There was a problem hiding this comment.
Fix variable name inconsistency
The variable name player_market_value seems to be a copy-paste error and should match the transfer context.
@router.get("/{player_id}/transfers", response_model=schemas.PlayerTransfers, response_model_exclude_none=True)
def get_player_transfers(player_id: str):
tfmkt = TransfermarktPlayerTransfers(player_id=player_id)
- player_market_value = tfmkt.get_player_transfers()
- return player_market_value
+ player_transfers = tfmkt.get_player_transfers()
+ return player_transfers📝 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.
| @router.get("/{player_id}/transfers", response_model=schemas.PlayerTransfers, response_model_exclude_none=True) | |
| def get_player_transfers(player_id: str): | |
| tfmkt = TransfermarktPlayerTransfers(player_id=player_id) | |
| player_market_value = tfmkt.get_player_transfers() | |
| return player_market_value | |
| @router.get("/{player_id}/transfers", response_model=schemas.PlayerTransfers, response_model_exclude_none=True) | |
| def get_player_transfers(player_id: str): | |
| tfmkt = TransfermarktPlayerTransfers(player_id=player_id) | |
| player_transfers = tfmkt.get_player_transfers() | |
| return player_transfers |
| "fromDate": date_from, | ||
| "untilDate": date_until, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing Pydantic models for response structure
While the key renaming to camelCase is good, given that this PR's objective is to implement Pydantic schemas, consider defining proper Pydantic models for the injury data structure instead of using raw dictionaries.
Here's a suggested implementation:
from pydantic import BaseModel
from typing import List
class PlayerInjury(BaseModel):
season: str
injury: str
fromDate: str
untilDate: str
days: str
gamesMissed: str
gamesMissedClubs: List[str]
class PlayerInjuriesResponse(BaseModel):
id: str
pageNumber: int
lastPageNumber: int
injuries: List[PlayerInjury]This would provide:
- Runtime type validation
- Automatic serialization/deserialization
- Self-documenting API schemas
|
|
||
| return clean_response(self.response) | ||
| return self.response |
There was a problem hiding this comment.
💡 Codebase verification
Schema validation mismatch detected between service and Pydantic model
The service's response includes fields that are not defined in the Pydantic schema:
- Missing
idfield - Missing nested
squaddictionary with its fields - Missing nested
leaguedictionary with its fields - Missing
historicalCrestsarray - Missing
confederationandfifaWorldRankingfields
Additionally, some field types in the service response might not match the schema:
current_transfer_recordandcurrent_market_valueare defined asintbut service returns raw textfounded_onis defined asdatebut service returns text
🔗 Analysis chain
LGTM: Clean response handling
Direct return of self.response aligns well with the PR objective of implementing Pydantic schemas, as the response validation will be handled by the schema layer instead of a custom cleaning function.
Let's verify the Pydantic schema implementation:
Let me gather more context about the response structure and its validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the corresponding Pydantic schema exists and validates this response structure
# Check for the schema file
fd "profile.py" "app/schemas"
# Look for the ClubProfile schema class
rg "class ClubProfile" -A 10
Length of output: 680
Script:
#!/bin/bash
# Check the service implementation to understand the response structure
rg -B 5 -A 5 "self.response" "app/services/clubs/profile.py"
# Look for any additional fields in the schema
rg "class ClubProfile" -A 20 "app/schemas/clubs/profile.py"
# Check the base model for any additional validation
rg "class TransfermarktBaseModel" -A 10
Length of output: 4700
| if not any(char.isdigit() for char in v): | ||
| return None | ||
| return int(v.replace(",", "").replace("m", "")) |
There was a problem hiding this comment.
Handle decimal values in parse_height correctly
Converting height strings like "1.78m" directly to int will cause a ValueError due to the decimal point. To accurately represent heights, use float instead of int.
Apply this diff to fix the conversion:
def parse_height(cls, v: str):
if not any(char.isdigit() for char in v):
return None
- return int(v.replace(",", "").replace("m", ""))
+ return float(v.replace(",", "").replace("m", ""))📝 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.
| if not any(char.isdigit() for char in v): | |
| return None | |
| return int(v.replace(",", "").replace("m", "")) | |
| if not any(char.isdigit() for char in v): | |
| return None | |
| return float(v.replace(",", "").replace("m", "")) |
felipeall
force-pushed
the
refactor/pydantic
branch
from
ab4ff8d to
bcd209a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/utils/utils.py (1)
137-140: Consider optimizing the camelCase conversionThe current implementation uses two separate list comprehensions. Consider combining them for better readability and performance.
Here's a suggested optimization:
- camel_case_headers = ["".join(word.capitalize() for word in header.split()) for header in headers] - camel_case_headers = [header[0].lower() + header[1:] for header in camel_case_headers] - - return [header for header in camel_case_headers] + return [ + "".join( + word.lower() if i == 0 else word.capitalize() + for i, word in enumerate(header.split()) + ) + for header in headers + ]This change:
- Combines the operations into a single list comprehension
- Avoids unnecessary intermediate list creation
- Reduces the number of string operations
app/schemas/base.py (1)
63-63: Remove redundant.replace(".", "")callsThe
.replace(".", "")method is called twice in the parsing logic. Removing the duplicate call cleans up the code without affecting functionality.Apply this diff to remove the redundant code:
v.replace("bn", "000000000") .replace("m", "000000") .replace("k", "000") .replace("€", "") - .replace(".", "") .replace("+", "") - .replace(".", "") .replace("'", "")Also applies to: 65-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
app/api/endpoints/clubs.py(1 hunks)app/api/endpoints/competitions.py(1 hunks)app/api/endpoints/players.py(2 hunks)app/schemas/base.py(1 hunks)app/schemas/clubs/__init__.py(1 hunks)app/schemas/clubs/players.py(1 hunks)app/schemas/clubs/profile.py(1 hunks)app/schemas/clubs/search.py(1 hunks)app/schemas/competitions/__init__.py(1 hunks)app/schemas/competitions/clubs.py(1 hunks)app/schemas/competitions/search.py(1 hunks)app/schemas/players/__init__.py(1 hunks)app/schemas/players/achievements.py(1 hunks)app/schemas/players/injuries.py(1 hunks)app/schemas/players/jersey_numbers.py(1 hunks)app/schemas/players/market_value.py(1 hunks)app/schemas/players/profile.py(1 hunks)app/schemas/players/search.py(1 hunks)app/schemas/players/stats.py(1 hunks)app/schemas/players/transfers.py(1 hunks)app/services/clubs/players.py(3 hunks)app/services/clubs/profile.py(2 hunks)app/services/clubs/search.py(0 hunks)app/services/competitions/clubs.py(1 hunks)app/services/competitions/search.py(0 hunks)app/services/players/achievements.py(3 hunks)app/services/players/injuries.py(3 hunks)app/services/players/jersey_numbers.py(2 hunks)app/services/players/market_value.py(3 hunks)app/services/players/profile.py(3 hunks)app/services/players/search.py(0 hunks)app/services/players/stats.py(2 hunks)app/services/players/transfers.py(3 hunks)app/utils/utils.py(1 hunks)pyproject.toml(2 hunks)requirements.txt(1 hunks)
💤 Files with no reviewable changes (3)
- app/services/competitions/search.py
- app/services/clubs/search.py
- app/services/players/search.py
🚧 Files skipped from review as they are similar to previous changes (29)
- app/schemas/competitions/init.py
- app/schemas/clubs/init.py
- app/services/competitions/clubs.py
- app/schemas/players/stats.py
- app/services/players/market_value.py
- app/services/clubs/profile.py
- app/schemas/competitions/search.py
- app/services/players/jersey_numbers.py
- app/schemas/clubs/search.py
- app/services/players/transfers.py
- app/schemas/players/init.py
- app/schemas/players/injuries.py
- app/schemas/players/search.py
- app/services/players/stats.py
- app/api/endpoints/competitions.py
- app/schemas/competitions/clubs.py
- app/schemas/players/achievements.py
- app/services/players/profile.py
- app/schemas/players/transfers.py
- app/services/players/achievements.py
- app/schemas/clubs/profile.py
- app/schemas/players/market_value.py
- app/schemas/players/jersey_numbers.py
- app/services/clubs/players.py
- app/schemas/players/profile.py
- app/api/endpoints/clubs.py
- app/schemas/clubs/players.py
- app/api/endpoints/players.py
- requirements.txt
🔇 Additional comments (8)
app/services/players/injuries.py (2)
60-61: LGTM! Good key renaming choices.
The renaming from "from"/"until" to "fromDate"/"untilDate" improves clarity and avoids potential conflicts with Python keywords.
Line range hint 1-84: Implement Pydantic models as per PR objective.
The current implementation using raw dictionaries doesn't align with the PR's objective of implementing Pydantic schemas.
The previous suggestion about implementing proper Pydantic models for injury data structure remains valid. This would provide:
- Runtime type validation
- Automatic serialization/deserialization
- Self-documenting API schemas
- Consistency with the PR's objective
Please refer to the previous review comment for the suggested Pydantic model implementation.
pyproject.toml (3)
41-42: Review ruff configuration changes
Several concerns about the ruff configuration:
- Enabling
previewandunsafe-fixesmight introduce instability - While adding
FASTrules is good for FastAPI projects, removingN(pep8-naming) might affect code consistency
Consider:
- Documenting why
unsafe-fixesis necessary - Re-enabling pep8-naming checks to maintain code consistency
Also applies to: 51-51
12-19: Verify compatibility between updated dependencies
Significant version updates detected:
- fastapi: 0.109.1 -> 0.115.5
- pydantic: newly added at 2.8.2
- pydantic-settings: 2.1.0 -> 2.6.1
Please ensure:
- FastAPI and Pydantic versions are compatible
- No breaking changes in FastAPI 0.115.5 affect the codebase
✅ Verification successful
Based on the shell script results, I can now generate the final response:
FastAPI and Pydantic usage is compatible with updated versions
The codebase follows standard FastAPI patterns that are compatible with the updated versions:
- Uses basic FastAPI features (routing, response models, HTTPException)
- Pydantic models follow the new v2 syntax with
model_config = ConfigDict - No deprecated features or patterns detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for FastAPI usage patterns that might be affected by version update
echo "Checking FastAPI patterns..."
rg -A 5 "from fastapi import|@router\."
# Check Pydantic model usage
echo "Checking Pydantic usage..."
rg -A 5 "from pydantic import|class.*BaseModel"
Length of output: 23081
3-3: Verify major version bump rationale
The version has been bumped from 2.0.3 to 3.0.0, indicating breaking changes. While implementing pydantic schemas could justify this, please ensure:
- All breaking changes are documented
- Migration guide is provided if needed
Also applies to: 8-8
app/utils/utils.py (2)
Line range hint 1-140: Well-structured utility module with comprehensive documentation
The module demonstrates good practices:
- Comprehensive docstrings with Args/Returns sections
- Consistent error handling and type hints
- Clear single-purpose utility functions
- Good separation of concerns
140-140: Verify the impact of removing special "Id" handling
The simplified return statement removes any special handling for "Id" suffixes. This change might affect API responses where "ID" was previously handled differently.
Let's verify the usage and impact:
✅ Verification successful
Removing special "Id" handling is safe to proceed
Based on the evidence from the codebase:
- The
to_camel_casefunction is used instats.pyandjersey_numbers.pyservices - The headers passed to this function already include "Id" in the correct format (e.g., "Competition id", "Club id", "Season id")
- The function's purpose is to convert space-separated or snake_case headers to camelCase, which it still does correctly
- No special handling for "Id" is required as the input headers already follow the desired convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential occurrences of camelCase fields ending with 'Id'
# to ensure this change doesn't break existing conventions
# Look for field definitions in Pydantic models
ast-grep --pattern 'class $_(BaseModel) { $$$ $id: $_ $$$ }'
# Search for direct dictionary key usage
rg -g '*.py' '["\'].*[iI]d["\']' app/
Length of output: 180
Script:
#!/bin/bash
# Search for potential occurrences of camelCase fields ending with 'Id'
# Look for field definitions in Pydantic models
ast-grep --pattern 'class $_(BaseModel) {
$$$
$id: $_
$$$
}'
# Search for direct dictionary key usage with Id/ID
rg -g '*.py' '["\']\w+[iI]d["\']' app/
# Look for the usage of to_camel_case function
rg -g '*.py' 'to_camel_case' app/
Length of output: 187
Script:
#!/bin/bash
# Look for direct dictionary key usage with Id/ID
rg -g '*.py' '["\'']\w+[iI]d["\'']\s*[:=]' app/
# Look for the usage of to_camel_case function with context
rg -g '*.py' -B 2 -A 2 'to_camel_case' app/
# Look at the full utils.py file to understand the context better
cat app/utils/utils.py
Length of output: 6104
app/schemas/base.py (1)
75-77: Handle decimal values in parse_height correctly
Converting height strings like "1.78m" directly to int will cause a ValueError due to the decimal point. To accurately represent heights, use float instead of int.
Apply this diff to fix the conversion:
def parse_height(cls, v: str):
if not any(char.isdigit() for char in v):
return None
- return int(v.replace(",", "").replace("m", ""))
+ return float(v.replace(",", "").replace("m", ""))| try: | ||
| return parser.parse(v).date() if v else None | ||
| except parser.ParserError: | ||
| return None |
There was a problem hiding this comment.
Fix exception handling in parse_str_to_date method
The dateutil.parser.parse function raises a ValueError when parsing fails, not parser.ParserError. Updating the exception handling ensures that parsing errors are correctly caught and handled.
Apply this diff to fix the exception handling:
def parse_str_to_date(cls, v: str):
try:
return parser.parse(v).date() if v else None
- except parser.ParserError:
+ except ValueError:
return None📝 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.
| try: | |
| return parser.parse(v).date() if v else None | |
| except parser.ParserError: | |
| return None | |
| try: | |
| return parser.parse(v).date() if v else None | |
| except ValueError: | |
| return None |
| parsed_value = ( | ||
| ( | ||
| v.replace("bn", "000000000") | ||
| .replace("m", "000000") | ||
| .replace("k", "000") | ||
| .replace("€", "") | ||
| .replace(".", "") | ||
| .replace("+", "") | ||
| .replace(".", "") | ||
| .replace("'", "") | ||
| ) | ||
| if v and any(char.isdigit() for char in v) | ||
| else None | ||
| ) | ||
| return int(parsed_value) if parsed_value else None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve parsing of decimal values in parse_str_to_int
The current implementation may incorrectly parse values with decimal points (e.g., "10.5m"). To accurately handle decimal values, consider retaining the decimal point and converting the string to a float before converting it to an integer.
Apply this diff to improve the parsing logic:
def parse_str_to_int(cls, v: str):
parsed_value = (
(
v.replace("bn", "e9")
.replace("m", "e6")
.replace("k", "e3")
.replace("€", "")
.replace("+", "")
.replace("'", "")
)
if v and any(char.isdigit() for char in v)
else None
)
- return int(parsed_value) if parsed_value else None
+ return int(float(parsed_value)) if parsed_value else NoneThis approach uses scientific notation to handle decimal values accurately.
📝 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.
| parsed_value = ( | |
| ( | |
| v.replace("bn", "000000000") | |
| .replace("m", "000000") | |
| .replace("k", "000") | |
| .replace("€", "") | |
| .replace(".", "") | |
| .replace("+", "") | |
| .replace(".", "") | |
| .replace("'", "") | |
| ) | |
| if v and any(char.isdigit() for char in v) | |
| else None | |
| ) | |
| return int(parsed_value) if parsed_value else None | |
| parsed_value = ( | |
| ( | |
| v.replace("bn", "e9") | |
| .replace("m", "e6") | |
| .replace("k", "e3") | |
| .replace("€", "") | |
| .replace("+", "") | |
| .replace("'", "") | |
| ) | |
| if v and any(char.isdigit() for char in v) | |
| else None | |
| ) | |
| return int(float(parsed_value)) if parsed_value else None |
felipeall
force-pushed
the
refactor/pydantic
branch
from
bcd209a to
4fedef1
Compare
felipeall
force-pushed
the
refactor/pydantic
branch
from
4fedef1 to
01e587b
Compare
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores