Fix most of type errors for src/api
#1150
Conversation
* unify `get_by_query` and `get_by_endpoint` methods into single `get` * remove unreachable according to type checkers code -- let exceptions rise and be explicit * improve types' usage correctness
📝 WalkthroughWalkthroughRefactors API parsing to DataApi TypedDicts, consolidates GET surface to unified get/get_parsed/get_converted, replaces many ParsedApiResponse/PreProcessedApiResponse usages with ParsedDataApiResponse and QueryOptions, and updates connectors, UI call sites, utils, models, and tests to the new signatures and flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI
participant Connector
participant ApiAccessor
participant Server
UI->>Connector: requestData(query: QueryOptions)
Connector->>ApiAccessor: get_parsed(query, response_handler)
ApiAccessor->>Server: HTTP GET /... with query
Server-->>ApiAccessor: 200 { data, included?, meta? } (DataApiResponse)
ApiAccessor->>ApiAccessor: parse_message(DataApiResponse) → ParsedDataApiResponse
ApiAccessor->>Connector: invoke response_handler(ParsedDataApiResponse)
Connector->>UI: emit converted domain models
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2026-01-02T02:53:05.593ZApplied to files:
📚 Learning: 2025-12-28T02:07:52.863ZApplied to files:
📚 Learning: 2026-01-02T02:50:32.067ZApplied to files:
📚 Learning: 2026-01-02T02:53:05.593ZApplied to files:
📚 Learning: 2026-01-02T02:40:22.985ZApplied to files:
🧬 Code graph analysis (1)src/api/ApiAccessors.py (6)
🪛 Ruff (0.14.10)src/api/ApiAccessors.py36-36: Undefined name (F821) 170-170: Do not catch blind exception: (BLE001) 171-171: Use Replace with (TRY400) 🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vaults/detailswidget.py (1)
379-391: Missing type assertion foritem_info.
ParsedDataApiResponse["data"]can be eitherdict[str, Any]orlist[dict[str, Any]]. This code accessesitem_info["versions"]assuming it's a dict, but there's no assertion to validate this. For consistency withon_review_submitted(line 165), add a type assertion.🔎 Proposed fix
def on_reviews_data(self, message: ParsedDataApiResponse) -> None: self._comments_initialized = True item_info = message["data"] + assert isinstance(item_info, dict) reviews: list[VersionReview] = [] for version in item_info["versions"]:src/api/player_api.py (1)
40-49: Potential KeyError if player has no clan membership.Line 44 accesses
player_dict["clanMembership"]without first checking if the key exists. If the player is not in a clan, this will raise aKeyError. Additionally, for consistency with other handlers in this PR (e.g.,handle_player_ratings,handle_achievements), consider assertingmessage["data"]is a list before unpacking.🔎 Proposed fix
def handle_player(self, message: ParsedDataApiResponse) -> None: + assert isinstance(message["data"], list) player_dict, = message["data"] - assert isinstance(player_dict, dict) player = Player(**player_dict) - if "clan" in player_dict["clanMembership"]: + if "clanMembership" in player_dict and "clan" in player_dict["clanMembership"]: clan = Clan(**player_dict["clanMembership"]["clan"]) clan_membership = ClanMembership(**player_dict["clanMembership"]) clan_membership.custom_clan = clan player.custom_clan_membership = clan_membership self.player_ready.emit(player)
🧹 Nitpick comments (9)
src/config/admin.py (2)
16-16: Unify Windows detection across the file.Line 16 now uses
sys.platform == "win32", but line 33 still usesos.name != 'nt'. For consistency, apply the same platform check throughout the file.🔎 Proposed fix for consistency
def runAsAdmin(cmdLine=None, wait=True): - if os.name != 'nt': + if sys.platform != "win32": raise RuntimeError("This function is only implemented on Windows.")Also applies to: 26-28, 33-34
14-28: Note: Changes appear out of scope for PR objectives.This PR is titled "Fix most of type errors for
src/api" and addresses issue #1149 about type hint consistency in API files. However,src/config/admin.pyis not an API file, and these changes (platform detection and f-string conversion) don't address type errors.While the changes themselves are reasonable refactoring improvements, consider whether they should be in a separate PR focused on general code modernization to keep PRs focused and easier to review.
src/model/game.py (1)
282-291: LGTM! Explicit None handling improves type safety.The refactored implementation correctly handles the case where
to_playerreturnsNone, making the None-filtering explicit and type-checker friendly. The logic is sound: gather potential players, filter outNonevalues, and compute the average or return 0 for empty lists.Optional: Combine steps for efficiency
If you'd like to reduce intermediate list creation, you could use a walrus operator to filter in a single comprehension:
- players = [ - self.to_player(name) - for team in self.playing_teams.values() - for name in team - ] - online_players = [p for p in players if p is not None] + online_players = [ + player + for team in self.playing_teams.values() + for name in team + if (player := self.to_player(name)) is not None + ]The current two-step approach is more readable, so this is purely optional.
src/fa/replayserver.py (1)
7-7: TheAnyimport suggests overly broad typing.This import is only necessary because line 80 uses
dict[str, Any]. According to the PR objectives, strongly-typed schemas likeDataApiResponsewere introduced to improve type safety. Consider using those instead ofAny.src/replays/_replayswidget.py (1)
1197-1211: Consider adding type assertion formessage["data"].Since
ParsedDataApiResponse["data"]can be eitherdictorlist, but line 1202 iterates overreplaysassuming it's a list, consider adding an assertion similar to other handlers in this PR for consistency and fail-fast behavior.Suggested defensive assertion
def process_replays_data(self, message: ParsedDataApiResponse) -> None: self.stopSearchVault() self.clear_scoreboard() self.onlineReplays = {} replays = message["data"] + assert isinstance(replays, list) for replay_item in replays:src/playercard/ratingtabwidget.py (1)
164-175: Consider usingParsedDataApiResponsefor stronger typing.The method accesses
message["meta"]andmessage["data"]/message["included"](viaLineSeriesParser), which matches theParsedDataApiResponsestructure. Usingdict[str, Any]works but loses the type guarantees thatParsedDataApiResponseprovides. If the signal emits this type, consider updating for consistency with the rest of the PR.src/api/ApiAccessors.py (1)
114-134: Side effect:message.pop('included')mutates the input.Line 130 mutates the input
messageby removing the'included'key. This could cause unexpected behavior if the caller retains a reference to the original response. Consider working with a copy or documenting this mutation clearly.src/api/stats_api.py (2)
182-184: Generator emitted via signal can only be iterated once.Line 184 emits a generator expression. If the signal receiver iterates over it more than once, the second iteration will yield no items. Consider converting to a list for safety.
🔎 Proposed fix
def handle_achievements(self, message: ParsedDataApiResponse) -> None: assert isinstance(message["data"], list) - self.achievments_ready.emit(PlayerAchievement(**entry) for entry in message["data"]) + self.achievments_ready.emit([PlayerAchievement(**entry) for entry in message["data"]])
191-196: Same generator pattern used inconvert_parsed.Similar to
handle_achievements, this returns a generator in"values". If the consumer iterates twice, the second pass will be empty. This may be intentional if the consumer is known to iterate only once, but worth noting for consistency with otherconvert_parsedmethods that return lists.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/api/ApiAccessors.pysrc/api/ApiBase.pysrc/api/coop_api.pysrc/api/featured_mod_api.pysrc/api/matchmaker_queue_api.pysrc/api/player_api.pysrc/api/sim_mod_updater.pysrc/api/stats_api.pysrc/api/vaults_api.pysrc/chat/ircconnection.pysrc/client/connection.pysrc/config/__init__.pysrc/config/admin.pysrc/connectivity/IceServersPoller.pysrc/fa/replayserver.pysrc/games/_gameswidget.pysrc/games/automatchframe.pysrc/model/game.pysrc/model/player.pysrc/model/qobjectmapping.pysrc/oauth/oauth_flow.pysrc/playercard/ratingtabwidget.pysrc/replays/_replayswidget.pysrc/stats/leaderboard_widget.pysrc/util/__init__.pysrc/util/gameurl.pysrc/util/theme.pysrc/vaults/detailswidget.pysrc/vaults/mapvault/mapdetails.pysrc/vaults/reviewwidget.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:57.647Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed intentionally raises NotImplementedError as a template method pattern - subclasses using get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed.
📚 Learning: 2025-12-28T16:29:14.773Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/sim_mod_updater.py:21-27
Timestamp: 2025-12-28T16:29:14.773Z
Learning: In src/api/sim_mod_updater.py, the assertion `assert isinstance(message["data"], list)` in the `get_url_from_message` method is intentional. If the API returns a dict instead of a list, the application should crash to signal an API contract violation rather than handling it gracefully.
Applied to files:
src/api/sim_mod_updater.py
📚 Learning: 2025-12-28T02:07:57.647Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:57.647Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed intentionally raises NotImplementedError as a template method pattern - subclasses using get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed.
Applied to files:
src/api/sim_mod_updater.pysrc/api/matchmaker_queue_api.pysrc/api/stats_api.pysrc/api/coop_api.pysrc/api/vaults_api.pysrc/api/featured_mod_api.pysrc/vaults/mapvault/mapdetails.py
📚 Learning: 2025-12-29T03:25:49.981Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/sim_mod_updater.py:21-27
Timestamp: 2025-12-29T03:25:49.981Z
Learning: In src/api/sim_mod_updater.py, treat a missing 'downloadUrl' in the API response as a hard API contract violation and fail fast. Do not swallow KeyError or provide fallback handling; allow the exception (or a clearly named custom exception) to crash the process, signaling an unmet contract. This should be explicit in code paths that access response['downloadUrl'], ensuring absence leads to immediate failure rather than silent degradation. If applicable, add a concise comment explaining the fail-fast rationale.
Applied to files:
src/api/sim_mod_updater.py
📚 Learning: 2025-11-23T00:07:39.893Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1145
File: src/api/vaults_api.py:114-116
Timestamp: 2025-11-23T00:07:39.893Z
Learning: In src/api/vaults_api.py, the MapPoolApiConnector class uses intentional stateful filter mutation. The request_pool_for_queue method sets self._filters and is the intended public API; request_data() should not be called directly on instances of this class.
Applied to files:
src/api/vaults_api.py
📚 Learning: 2025-12-28T02:07:52.863Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:52.863Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed should be a template method that raises NotImplementedError. Subclasses that use get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed to provide concrete parsing logic. This enforces that derived accessors supply the needed parsing behavior and prevents accidental use of a base implementation.
Applied to files:
src/api/ApiAccessors.py
📚 Learning: 2025-12-27T22:23:49.101Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/fa/game_updater/updater.py:274-279
Timestamp: 2025-12-27T22:23:49.101Z
Learning: In src/fa/game_updater/updater.py, the single-element unpacking `mod, = data["values"]` in the `process_fmod` method is intentional and serves as validation that exactly one featured mod is returned. Multiple or zero values should raise a ValueError to signal an API error.
Applied to files:
src/api/featured_mod_api.py
🧬 Code graph analysis (20)
src/connectivity/IceServersPoller.py (1)
src/api/ApiBase.py (2)
get(72-82)get(140-153)
src/replays/_replayswidget.py (1)
src/api/ApiAccessors.py (2)
ParsedDataApiResponse(45-47)get_parsed(61-71)
src/games/_gameswidget.py (1)
src/games/automatchframe.py (2)
MatchmakerQueueFrame(28-253)handleQueueInfo(123-131)
src/api/sim_mod_updater.py (1)
src/api/ApiAccessors.py (2)
ParsedDataApiResponse(45-47)get_parsed(61-71)
src/stats/leaderboard_widget.py (1)
src/api/ApiAccessors.py (2)
ParsedDataApiResponse(45-47)get_parsed(61-71)
src/vaults/reviewwidget.py (2)
src/api/ApiAccessors.py (1)
ParsedDataApiResponse(45-47)src/vaults/detailswidget.py (1)
on_reviews_data(379-391)
src/api/matchmaker_queue_api.py (2)
src/api/ApiAccessors.py (1)
ParsedDataApiResponse(45-47)src/api/models/MatchmakerQueue.py (1)
MatchmakerQueue(10-15)
src/api/player_api.py (1)
src/api/ApiAccessors.py (2)
ParsedDataApiResponse(45-47)get_parsed(61-71)
src/vaults/detailswidget.py (3)
src/api/ApiAccessors.py (1)
ParsedDataApiResponse(45-47)src/vaults/mapvault/mapdetails.py (1)
allow_review(55-58)src/vaults/reviewwidget.py (1)
on_reviews_data(330-337)
src/api/stats_api.py (1)
src/api/ApiAccessors.py (2)
ParsedDataApiResponse(45-47)get_parsed(61-71)
src/model/player.py (1)
src/model/game.py (1)
Game(44-302)
src/games/automatchframe.py (1)
src/api/models/MatchmakerQueue.py (1)
MatchmakerQueue(10-15)
src/model/game.py (3)
src/chat/chatter_model.py (1)
name(187-191)src/model/qobjectmapping.py (1)
values(61-62)src/model/player.py (1)
global_estimate(73-74)
src/api/coop_api.py (3)
src/api/ApiAccessors.py (1)
ParsedDataApiResponse(45-47)src/api/models/CoopScenario.py (1)
CoopScenario(7-13)src/api/models/CoopResult.py (1)
CoopResult(7-14)
src/chat/ircconnection.py (1)
src/api/ApiBase.py (2)
get(72-82)get(140-153)
src/oauth/oauth_flow.py (1)
src/config/__init__.py (5)
Settings(43-207)get(59-69)get_list(73-77)get_list(81-85)get_list(88-151)
src/api/vaults_api.py (3)
src/api/ApiAccessors.py (4)
DataApiAccessor(50-189)ParsedDataApiResponse(45-47)convert_parsed(188-189)get_parsed(61-71)src/api/featured_mod_api.py (2)
convert_parsed(18-23)convert_parsed(34-39)src/api/matchmaker_queue_api.py (1)
convert_parsed(14-19)
src/api/ApiAccessors.py (4)
src/api/ApiBase.py (4)
handle(44-45)_do_nothing(29-30)get(72-82)get(140-153)src/api/coop_api.py (2)
convert_parsed(15-20)convert_parsed(65-72)src/api/matchmaker_queue_api.py (1)
convert_parsed(14-19)src/api/vaults_api.py (3)
convert_parsed(79-84)convert_parsed(100-108)convert_parsed(126-133)
src/api/featured_mod_api.py (3)
src/api/ApiAccessors.py (3)
ParsedDataApiResponse(45-47)requestData(177-186)get_converted(95-105)src/api/models/FeaturedMod.py (1)
FeaturedMod(6-12)src/api/models/FeaturedModFile.py (1)
FeaturedModFile(6-15)
src/vaults/mapvault/mapdetails.py (2)
src/api/ApiAccessors.py (2)
ParsedDataApiResponse(45-47)get_parsed(61-71)src/vaults/detailswidget.py (1)
allow_review(90-91)
🪛 GitHub Actions: Checks
src/util/theme.py
[error] 32-32: ValueError in ThemeSet._nullcheck: too many values to unpack (expected 1, got 2)
🪛 Ruff (0.14.10)
src/util/theme.py
17-17: Undefined name Theme
(F821)
18-18: Undefined name Theme
(F821)
29-29: Undefined name ThemeSet
(F821)
30-30: Undefined name ThemeSet
(F821)
src/api/ApiAccessors.py
36-36: Undefined name DataApiResponse
(F821)
168-168: Do not catch blind exception: Exception
(BLE001)
169-169: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (63)
src/chat/ircconnection.py (1)
196-200: LGTM! Correctly adopts the unified API method.The change from
get_by_endpointtogetaligns with the PR's goal of consolidating API access methods. The endpoint path, callback signatures, and error handling remain correct and unchanged.src/util/gameurl.py (2)
69-69: LGTM: Explicit type conversion improves clarity.The explicit
int()cast makes the type conversion visible and clear. The error handling is preserved sinceValueErrorfrom invalid integer strings is caught by the exception handler at line 78.
88-91: LGTM: Simplified signature improves maintainability.The refactor removes the generic type parameter and moves type conversion responsibility to the caller. This makes the code simpler and more explicit about conversions, which improves type safety and readability.
src/config/admin.py (1)
48-51: LGTM! f-string conversion improves readability.The migration from
.format()to f-strings is a clean modernization that improves code readability while maintaining the same functionality.src/model/player.py (1)
150-153: LGTM! Clean consolidation of duplicate setters.The property setter correctly delegates to
set_currentGame, and the CAVEAT comment appropriately warns that assignments likeplayer.currentGame = gamewill emit signals immediately (since no transaction context is passed). This design provides both convenience (property setter) and control (transactional method for batched updates).src/util/theme.py (1)
110-110: LGTM: Correct method name for Python file API.The change from
readLines()toreadlines()correctly aligns with Python's standard file API.src/fa/replayserver.py (1)
78-78: Method signature is compatible—no changes needed.The
get()method signature fromJsonApiBasecorrectly handles the endpoint path as a string and properly wraps the response handler to decode theQNetworkReplyintodict[str, Any], matching the expectations of bothon_replay_access_urlandon_api_errorcallbacks. The consolidation ofget_by_endpointintoget()is complete and correct.src/config/__init__.py (2)
38-38: LGTM!The type annotation for
_unpersisted_settingscorrectly matches the key types used inSettings.set(lines 154-158) andSettings.contains(line 206), ensuring type consistency across the Settings class.
307-307: LGTM!The explicit type annotation and consolidated initialization improve clarity. The type correctly reflects the return type of
_settings.valuewithtype=str.src/oauth/oauth_flow.py (5)
1-3: LGTM!The imports support the new type annotations and follow standard typing conventions.
29-29: LGTM!The
ClassVar[Logger]annotation correctly declares the logger attribute injected by the@with_loggerdecorator and matches its usage in the lifecycle callbacks (lines 72, 76, 79, 83).
54-54: LGTM!The type annotation correctly reflects that
_expires_incan beNone(when expiration checking is stopped) orint(milliseconds until expiration), matching its usage incheck_token(line 64) andon_expiration_at_changed(line 73).
56-59: Good improvement to state management.Resetting
_expires_intoNonewhen stopping expiration checks improves correctness by ensuring stale expiration times don't persist. This aligns with the guard incheck_token(line 64) that treatsNoneas "not checking."
86-104: LGTM!The explicit defaults improve type safety:
- Line 90:
default=""ensuresclient_idis strictlystrrather thanstr | None- Line 91:
default=[]ensuresscopesis strictlylist[str]rather thanlist[str] | NoneBoth changes leverage the appropriate
Settings.getoverloads and improve type checker support.src/api/ApiBase.py (2)
72-82: Good unification of query and endpoint-based GET requests.The consolidated
getmethod elegantly handles bothQueryOptions(dict-like) andstr(endpoint path) inputs through runtime type checking, reducing code duplication while maintaining type safety.
23-23: No action needed. The project is configured to use Python 3.14 in all build workflows, which fully supports thetypestatement syntax (available since Python 3.12). The code is compatible with the project's Python version.src/connectivity/IceServersPoller.py (1)
21-24: Acknowledged: Response type mismatch is a known issue.The
type: ignorewith FIXME appropriately documents that/ice/session/game/{id}returns a non-standard response shape. The migration toget()is correct; the underlying type inconsistency is a separate concern to address later.src/games/_gameswidget.py (1)
445-453: Good refactor extractingteam_sizeto a local variable.The extraction improves readability and ensures consistency across the insert position, queue comparison, constructor call, and tab naming. The change correctly aligns with the
MatchmakerQueueFrameconstructor signature shown in the relevant snippets.src/vaults/reviewwidget.py (1)
20-20: Type migration toParsedDataApiResponseis correct.The import and type hint changes align with the new API response structure. The existing assertion at line 334 properly validates that
datais a list before indexing, which is necessary sinceParsedDataApiResponse["data"]can be either a dict or list.Also applies to: 330-337
src/vaults/mapvault/mapdetails.py (2)
36-46: Migration toget_parsedwith correct query structure.The method call correctly uses the unified
get_parsedAPI with a query dict. The query parameters are properly typed as string values compatible withQueryOptions.
55-58: Type consistency with parent class confirmed.The
allow_reviewoverride correctly usesParsedDataApiResponse, matching the parent class signature shown insrc/vaults/detailswidget.py(lines 89-90). Thelen(response["data"])check works correctly sincedatacan be a dict or list, both supportinglen().src/api/sim_mod_updater.py (2)
22-28: Intentional fail-fast behavior preserved.Based on learnings, the assertion at line 23 and the potential
KeyErrorat line 25 are intentional design choices to signal API contract violations immediately rather than degrading silently. The type migration toParsedDataApiResponseis correct.
31-32: Correct migration toget_parsedwith explicitQueryOptionstyping.The explicit type annotation
query: QueryOptionsimproves readability and aligns with the new unified API access pattern.src/replays/_replayswidget.py (1)
943-947: Correct migration to unifiedget_parsedmethod.The API call correctly uses the new
get_parsedmethod with query parameters, response handler, and error handler arguments matching the expected signature.src/vaults/detailswidget.py (2)
18-18: LGTM!Import updated correctly to use the new
ParsedDataApiResponsetype.
163-165: Good addition of runtime assertion.The
assert isinstance(data, dict)properly validates the data structure before accessing type-specific keys likedata["type"]anddata["player"].src/stats/leaderboard_widget.py (3)
7-7: LGTM!Import updated correctly.
163-167: LGTM!Signature correctly updated to use
ParsedDataApiResponse, and the method properly accessesparsed["meta"].
314-314: LGTM!API call correctly updated to use
get_parsed.src/playercard/ratingtabwidget.py (1)
1-2: LGTM!Import added for type annotation.
src/games/automatchframe.py (3)
14-14: LGTM!Import of
MatchmakerQueuemodel added to support the new typed API response handling.
28-28: Good rename to avoid naming conflict.Renaming the class to
MatchmakerQueueFrameavoids confusion with the importedMatchmakerQueuemodel.
117-121: LGTM!Good use of the typed
MatchmakerQueuemodel with proper assertion before accessing the optionalleaderboardfield. The iteration now uses object attributes (queue.name,queue.leaderboard.technical_name) instead of dictionary keys.src/api/matchmaker_queue_api.py (1)
14-19: LGTM!The
convert_parsedimplementation correctly:
- Uses
ParsedDataApiResponseparameter type- Adds runtime assertion to validate data is a list
- Returns properly typed
dict[str, list[MatchmakerQueue]]- Constructs
MatchmakerQueuemodel instances from raw dataThis follows the template method pattern as expected.
src/api/featured_mod_api.py (5)
6-7: LGTM!Imports updated correctly for the new types.
18-23: LGTM!
convert_parsedcorrectly updated withParsedDataApiResponsetype and runtime assertion.
25-27: LGTM!Good explicit type annotation for the query variable using
QueryOptions.
34-39: LGTM!
convert_parsedcorrectly updated with assertion and proper return type.
45-49: LGTM!Methods correctly updated to use
get_convertedwith endpoint paths and signal emitters as handlers.src/api/coop_api.py (6)
2-3: LGTM!Imports updated correctly for the new types.
15-20: LGTM!
convert_parsedcorrectly updated with assertion and proper typing.
27-33: LGTM!Good rename from
prepare_query_dicttoprepare_query_optionsto align with theQueryOptionstype.
35-38: LGTM!The
str(cur_filters)cast safely handles the case wherefilterkey might be missing (.get()returns empty string).
48-63: LGTM!Good defensive coding with:
- Explicit type annotations for
unique_resultsandunique_teams- Runtime assertions for
result.gameandresult.game.player_stats- Proper filtering of
Noneplayers before creating the sorted tuple
65-72: LGTM!
convert_parsedcorrectly updated with assertion and proper return type.src/api/player_api.py (2)
22-31: LGTM!The method correctly uses the new
QueryOptionstype and migrates toget_parsedwith direct signal emission as the handler.
33-38: LGTM!Consistent migration to
QueryOptionstyping andget_parsedmethod.src/api/vaults_api.py (6)
38-40: LGTM!Using
copy()from the standard library is appropriate for shallow copying the query options dictionary.
79-84: LGTM!The runtime assertion ensures type safety before iterating over
parsed["data"], consistent with theParsedDataApiResponsetype definition wheredatacan be either a dict or list.
100-108: LGTM!Consistent pattern with assertion guard before list iteration.
126-133: LGTM!Properly updated signature and assertion pattern.
142-145: LGTM!The migration from
get_by_query_parsedtoget_parsedis correct, and emitting directly viaself.data_ready.emitis appropriate for this use case.
179-188: LGTM!Handler type correctly updated to
ParsedDataApiResponse.src/api/ApiAccessors.py (4)
53-71: LGTM!The
_handler_parsedandget_parsedmethods are well-structured, properly typed, and provide a clean API for handling parsed responses.
87-105: LGTM!The
_handler_convertedandget_convertedmethods follow the same pattern as the parsed variants, maintaining consistency.
107-112: LGTM!The
parse_messagemethod correctly composes the parsing steps and returns a properly typedParsedDataApiResponse.
188-189: Intentional template method pattern.This follows the established pattern where subclasses using
get_convertedorrequestDatamust implementconvert_parsed. Based on learnings from previous reviews, this is the intended design.src/api/stats_api.py (5)
38-44: LGTM!Consistent pattern with assertion guard and proper type annotation for
ParsedDataApiResponse.
51-61: LGTM!The
convert_parsedmethod correctly asserts list type and returns a properly structured result.
122-134: LGTM!The handler correctly extracts and validates
score_databefore conditional emission.
136-147: LGTM!Consistent pattern with assertion and proper migration to
get_parsed.
163-165: LGTM!Properly updated handler with type-safe assertion.
src/client/connection.py (1)
260-264: The method change fromget_by_endpointtogetis correct. The callback signaturedict[Literal["accessUrl"], str]is compatible with thegetmethod'sresponse_handler: Callable[[dict[str, Any]], None]parameter, and usingget(rather thanget_parsed) is appropriate since the/lobby/accessendpoint returns a simple JSON structure, not JSON:API format requiring parsing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/ApiAccessors.py (1)
168-170: Uselogger.exceptionfor better error diagnostics.While the typo has been corrected, the logging should use
logger.exceptioninstead oflogger.errorto automatically capture the stack trace, which is valuable for debugging parsing errors.🔎 Proposed fix
except Exception as e: - logger.error("Error parsing %s: %s", data, e) + logger.exception("Error parsing %s", data)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/api/ApiAccessors.pysrc/connectivity/IceServersPoller.pysrc/replays/zigparser/parser.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/connectivity/IceServersPoller.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Gatsik
Repo: FAForever/client PR: 1150
File: src/fa/replayserver.py:80-80
Timestamp: 2026-01-02T02:50:24.628Z
Learning: In the FAForever client codebase, all API response handlers (callbacks passed to `get`, `post`, etc. in ApiBase) must accept `dict[str, Any]` as their parameter type to maintain signature compatibility. More specific types like `dict[Literal["key"], type]` or custom TypedDicts cannot be used directly in handler signatures due to contravariance constraints.
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:57.647Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed intentionally raises NotImplementedError as a template method pattern - subclasses using get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed.
📚 Learning: 2026-01-02T02:53:05.593Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1150
File: src/api/ApiAccessors.py:32-42
Timestamp: 2026-01-02T02:53:05.593Z
Learning: In the FAForever client (Python 3.14+), forward references in TypedDict definitions (e.g., DataApiResourceObject referencing DataApiResponse defined later in src/api/ApiAccessors.py) are supported and should not be flagged as errors by static analysis tools. Treat forward references in TypedDicts as valid; ensure type checkers (e.g., mypy) are configured to allow PEP 695/Typing enhancements and that such patterns pass type checks without false positives.
Applied to files:
src/api/ApiAccessors.py
📚 Learning: 2025-12-28T02:07:52.863Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1148
File: src/api/ApiAccessors.py:81-88
Timestamp: 2025-12-28T02:07:52.863Z
Learning: In src/api/ApiAccessors.py, DataApiAccessor.convert_parsed should be a template method that raises NotImplementedError. Subclasses that use get_by_query_converted, get_by_endpoint_converted, or requestData must implement convert_parsed to provide concrete parsing logic. This enforces that derived accessors supply the needed parsing behavior and prevents accidental use of a base implementation.
Applied to files:
src/api/ApiAccessors.py
📚 Learning: 2026-01-02T02:53:05.593Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1150
File: src/api/ApiAccessors.py:32-42
Timestamp: 2026-01-02T02:53:05.593Z
Learning: In FAForever/client review context, for Python files (e.g., src/api/ApiAccessors.py), do not mention Ruff-specific warnings or propose Ruff configuration changes. Use generic lint guidance or project-approved lints instead; avoid any Ruff-related recommendations in reviews.
Applied to files:
src/api/ApiAccessors.py
📚 Learning: 2026-01-02T02:50:24.628Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1150
File: src/fa/replayserver.py:80-80
Timestamp: 2026-01-02T02:50:24.628Z
Learning: In the FAForever client codebase, all API response handlers (callbacks passed to `get`, `post`, etc. in ApiBase) must accept `dict[str, Any]` as their parameter type to maintain signature compatibility. More specific types like `dict[Literal["key"], type]` or custom TypedDicts cannot be used directly in handler signatures due to contravariance constraints.
Applied to files:
src/api/ApiAccessors.py
📚 Learning: 2026-01-02T02:40:22.985Z
Learnt from: Gatsik
Repo: FAForever/client PR: 1150
File: src/util/__init__.py:0-0
Timestamp: 2026-01-02T02:40:22.985Z
Learning: In the FAForever/client repository (Python), os.path.getmtime() can accept DirEntry objects returned by os.scandir() directly. Do not extract the path attribute when using getmtime as a sorting key; pass the DirEntry (which is path-like) to getmtime and sort on that value. This applies to all Python files in the repo, not just a single file, and should be validated against the supported Python version in use.
Applied to files:
src/api/ApiAccessors.py
🧬 Code graph analysis (1)
src/api/ApiAccessors.py (1)
src/api/ApiBase.py (4)
handle(44-45)_do_nothing(29-30)get(72-82)get(140-153)
🪛 Ruff (0.14.10)
src/api/ApiAccessors.py
36-36: Undefined name DataApiResponse
(F821)
168-168: Do not catch blind exception: Exception
(BLE001)
169-169: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (5)
src/replays/zigparser/parser.zig (1)
25-25: LGTM - Function signature corrected.The function signature correctly uses
ParseErrorinstead of the misspelledParseErorr. Once the duplicate declaration issue is resolved, this change properly aligns the function signature with the correct error type.src/api/ApiAccessors.py (4)
32-47: LGTM - Well-structured TypedDict definitions.The new TypedDict definitions for
DataApiResourceObject,DataApiResponse, andParsedDataApiResponseprovide excellent type safety for the Data API response structure. The use ofNotRequiredfor optional fields andLiteral["page"]for the meta structure demonstrates careful attention to the API contract.
53-59: LGTM - Handler refactoring improves type safety.The refactored handler methods (
_handler_parsedand_handler_converted) properly maintain signature compatibility while introducing strong typing throughcast(). This approach aligns with the codebase requirements where API handlers must acceptdict[str, Any]while providing improved type safety internally.Also applies to: 87-93
61-71: LGTM - Unified get methods improve API consistency.The refactored
get_parsedandget_convertedmethods provide a cleaner, more unified interface that accepts eitherQueryOptionsor string paths. The method naming is more concise while maintaining clarity about their parsing behavior.Also applies to: 95-105
107-145: LGTM - Parsing and accessor methods properly refactored.The parsing methods (
parse_message,parseIncluded,parse_data,parse_single,parse_meta) are well-structured with appropriate type annotations using the new TypedDict types. TherequestDatamethod correctly uses the renamedget_convertedmethod, andconvert_parsedappropriately serves as a template method requiring subclass implementation.Also applies to: 172-189
1 participant
Resolves #1149
Summary by CodeRabbit
Refactoring
Stability / Bug Fixes
User-facing behavior
✏️ Tip: You can customize this high-level summary in your review settings.