Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the headerless-root crash and incorrect inexact-match classification.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors Match2 into focused match, bracket, input, display, and type modules while preserving compatibility exports.
Changes:
- Extracts domain-specific utilities and model types.
- Updates wiki consumers to use shared input helpers.
- Adds unit coverage for refactored behavior.
File summaries
| File | Reviewed change |
|---|---|
lua/wikis/valorant/MatchGroup/Input/Custom.lua |
Uses shared input helpers. |
lua/wikis/leagueoflegends/MatchGroup/Input/Custom.lua |
Uses shared input helpers. |
lua/wikis/dota2/MatchGroup/Input/Custom.lua |
Uses shared input helpers. |
lua/wikis/commons/MatchGroup/Util/Types.lua |
Defines shared model types. |
lua/wikis/commons/MatchGroup/Util/Starcraft.lua |
Adapts StarCraft parsing. |
lua/wikis/commons/MatchGroup/Util/Match.lua |
Extracts match conversion logic; date classification requires correction. |
lua/wikis/commons/MatchGroup/Util/Bracket.lua |
Extracts bracket logic. |
lua/wikis/commons/MatchGroup/Util.lua |
Preserves aggregate utilities and exports. |
lua/wikis/commons/MatchGroup/Input/Util.lua |
Adds input normalization helpers. |
lua/wikis/commons/MatchGroup/Input.lua |
Uses input normalization. |
lua/wikis/commons/MatchGroup/Display/Helper.lua |
Handles bracket-reset merging. |
lua/wikis/commons/MatchGroup/Display/Bracket.lua |
Uses display merging helpers. |
lua/wikis/commons/MatchGroup/Coordinates.lua |
Headerless roots still cause a crash. |
lua/wikis/commons/Match.lua |
Uses input normalization. |
lua/spec/match_group_util_spec.lua |
Tests utility assembly and exports. |
lua/spec/match_group_util_match_spec.lua |
Tests match model behavior. |
lua/spec/match_group_util_bracket_spec.lua |
Tests bracket behavior. |
lua/spec/match_group_input_util_spec.lua |
Tests input helpers. |
lua/spec/match_group_display_helper_spec.lua |
Tests display merging. |
Review details
Suppressed comments (1)
lua/wikis/commons/MatchGroup/Util/Match.lua:246
- This preserves a real classification bug:
matchFromRecordconvertsdateexact = '0'intodateIsExact = false, butmatch.dateIsExact or match.dateexactdiscards that explicitfalse, so a past inexact match is reported asongoing. SelectdateIsExactwhen it is non-nil (includingfalse) and update the new regression test to expectupcomingfor the model path.
-- TODO: a dateIsExact of false becomes nil here, so it is treated as unknown rather than as
-- inexact, and a past match with dateIsExact = false reads as ongoing instead of upcoming.
-- Only the record spelling (dateexact = '0') currently makes a match inexact.
local isExact = Logic.readBoolOrNil(match.dateIsExact or match.dateexact)
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Conflict in lua/wikis/commons/MatchGroup/Util.lua: main changed three things in code this branch moved out. Kept the slim re-export version and migrated each change to its new home: - ---@field roles string[]? (#8016) -> standardPlayer in Util/Types.lua - ---@field submatches MatchGroupUtilSubgroup[] (#7978) -> MatchGroupUtilMatch in Util/Types.lua - match.submatches = groupBySubgroup(match) (#7978) -> MatchUtil.matchFromRecord in Util/Match.lua, via MatchUtil rather than the re-export That last one is load bearing: Util/Starcraft.lua was changed in #7978 to read match.submatches instead of recomputing it, so dropping the assignment would have left starcraft submatches empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rathoz
added this pull request to stack #8098
September 16, 2026 12:04
Rathoz
marked this pull request as ready for review
September 16, 2026 12:12
ElectricalBoy
left a comment
Collaborator
There was a problem hiding this comment.
seems reasonable for the most part
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split one of the massive of Match2 modules (MatchGroup/Util) into a few smaller ones along bracket/domain split.
The domainification will contain additional splitting.
The tests are temporary to help with migration. Let's make new tests in the end that tests intended functioanlity, rather than what the code correctly does..
How did you test this change?
tested on LoL bracket, matchlist and matchpage.