Port All_Filtered.json generator#420
Open
D1firehail wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
👮 Files not reviewed due to content moderation or server errors (2)
📝 WalkthroughWalkthroughA new executable project, AllFilteredGenerator, was added. It parses an All.json array to extract Prime equipment and Relic data, applies parsing/synchronization logic across domain models, and emits a consolidated JSON output with errors, timestamp, relic groupings, equipment, and ignored items. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (Program.Main)
participant Parser as JSON Parser
participant Prime as PrimeEquipment / Components
participant Relic as Relic / Forma sources
participant Sync as Synchronizers
participant Output as File Output
CLI->>Parser: Read All.json (JsonArray)
Parser->>Prime: Parse Prime entries (TryParse)
Prime->>Prime: Parse PrimeComponent & IndependentPrimeComponent
Parser->>Relic: Parse Relic entries (TryParse)
Relic->>Relic: Extract Forma drops (ItemRelicInfo)
CLI->>Sync: SyncRelicsWithParts(primes, relics, errors)
Sync->>Prime: Match parts -> relics, update part.Vaulted & prime.Vaulted
Sync->>Relic: Populate DropsByRarity
CLI->>Sync: SyncRelicsWithForma(formaInfo, relics, errors)
Sync->>Relic: Match forma sources, add "Forma Blueprint" entries
CLI->>Relic: DivideByEra(relics, errors)
Relic->>Relic: Group relics by EraName and validate counts
CLI->>Output: Build final JsonObject (errors, timestamp, relics, eqmt, ignored_items)
Output->>Output: Serialize & write file
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
AllFilteredGenerator/PrimeComponent.cs (1)
25-62:TryParsealways returns non-null despite nullable return type.The method signature indicates it can return
null(PrimeComponent?), but the implementation always returns a valid instance. Either:
- Remove the nullable annotation if the method should always succeed, or
- Return
nullwhen critical data is missing (e.g., when bothducatsandprimeSellingPriceare missing)Option 2: Return null when ducat value is missing
else { - savedDucatCount = 0; - errors.Add(objectName + " " + componentName + " IS MISSING ducats/primeSellingPrice"); + errors.Add(objectName + " " + componentName + " IS MISSING ducats/primeSellingPrice"); + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/PrimeComponent.cs` around lines 25 - 62, TryParse currently always constructs and returns a PrimeComponent despite its nullable signature; either remove the nullable or return null when critical ducat data is absent—preferably implement Option 2: detect missing ducat/primeSellingPrice (when parsedDucats and parsedPrimeSellingPrice are both null), log the error (errors.Add) and return null instead of creating a PrimeComponent; update the TryParse method so the branch where you set savedDucatCount to 0 and call errors.Add yields an immediate return null, leaving PrimeComponent(...) construction only in the success path.AllFilteredGenerator/ItemRelicInfo.cs (1)
67-71: Redundantelseblock.The
else { continue; }is unnecessary since the loop naturally continues to the next iteration.Proposed fix
result.Add(new ItemRelicInfo(eraName, nameInEra, rarity)); } - else - { - continue; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/ItemRelicInfo.cs` around lines 67 - 71, In the loop inside ItemRelicInfo (remove the redundant else { continue; } block): delete the else branch so the loop simply proceeds naturally when the if condition fails; keep the code path for the if branch intact and ensure subsequent statements remain at the same level (no further changes to method or variable names required).AllFilteredGenerator/Program.cs (2)
41-44: Inconsistent error handling: throws exception vsEnvironment.Exit.Missing arguments and missing file trigger
Environment.Exit(1), but invalid JSON structure throws an exception. Consider using a consistent approach.Proposed fix for consistency
if (json is null || json.GetValueKind() is not JsonValueKind.Array) { - throw new ArgumentException("All.json not a json array"); + Console.WriteLine("All.json is not a valid JSON array"); + Environment.Exit(1); + return; // Unreachable but satisfies compiler }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/Program.cs` around lines 41 - 44, The code currently throws an ArgumentException when the parsed JSON is null or not an array (in the block checking json is null or json.GetValueKind() is not JsonValueKind.Array), which is inconsistent with other error handling that calls Environment.Exit(1) for missing args/file; change this to use the same exit pattern: log or write a clear error message (matching other messages for missing args/file) and call Environment.Exit(1) instead of throwing, ensuring the message identifies the file/condition and locating the change in the JSON validation block (the if checking json is null || json.GetValueKind() is not JsonValueKind.Array).
177-189: Remove commented-out dead code.Commented-out code without explanation adds noise. If this is kept for reference, consider adding a comment explaining why it's preserved, or remove it entirely since version control maintains history.
Proposed cleanup
else { - // old code, doesn't handle 2X Forma Blueprint - //var category = elemObj.GetStringProperty("category"); - //if (category is not null && category.Equals("Misc", StringComparison.InvariantCultureIgnoreCase) && name.Equals("Forma")) - //{ - // var formaSources = ItemRelicInfo.TryParseDrops(elemObj); - // if (formaSources.Count > 0) - // { - // formaRelicInfo.AddRange(formaSources); - // } - //} + // Items that don't match Prime or Relic criteria are skipped }Or simply remove the else block entirely since it does nothing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/Program.cs` around lines 177 - 189, Remove the dead commented-out code inside the else block that references elemObj.GetStringProperty("category"), name.Equals("Forma"), ItemRelicInfo.TryParseDrops and formaRelicInfo — either delete the entire empty else block or replace it with a brief explanatory comment if you intentionally preserved it for historical reasons; ensure no commented legacy logic remains in Program.cs so only active code or a clear rationale comment remains.AllFilteredGenerator/IndependentPrimeComponent.cs (1)
21-28: Method nameTryParseis misleading since it never returns null.Unlike
PrimeComponent.TryParsewhich can returnnull, this method always returns a validIndependentPrimeComponent. Consider renaming toParseor changing the return type toIndependentPrimeComponent?if there are conditions under which parsing should fail.Proposed rename
- public static IndependentPrimeComponent TryParse(string componentName, JsonObject componentObj) + public static IndependentPrimeComponent Parse(string componentName, JsonObject componentObj)Also update the call site in
PrimeEquipment.cs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/IndependentPrimeComponent.cs` around lines 21 - 28, The method IndependentPrimeComponent.TryParse always returns a non-null IndependentPrimeComponent, so rename TryParse to Parse (change method name to Parse in the IndependentPrimeComponent class) and update all call sites (notably the usage in PrimeEquipment.cs) to call IndependentPrimeComponent.Parse; ensure the method signature and any XML comments are updated accordingly and run a solution-wide rename to catch any other references.AllFilteredGenerator/PrimeEquipment.cs (1)
29-45: Pre-index prime names before syncing independent parts.This loop does a full
primes.FirstOrDefault(...)per independent part. Building one case-insensitive map once will reduce repeated scans and simplify the method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/PrimeEquipment.cs` around lines 29 - 45, In SyncIndependentParts, avoid repeated linear scans by prebuilding a case-insensitive lookup (e.g., Dictionary<string, PrimeEquipment> using StringComparer.InvariantCultureIgnoreCase) mapping prime.Name to the PrimeEquipment, then replace primes.FirstOrDefault(...) with a dictionary.TryGetValue on that map; when a match is found preserve the existing logic (set part.Vaulted = matchingItem.Vaulted and prime.Vaulted |= matchingItem.Vaulted) and when not found add the error as before.AllFilteredGenerator/Relic.cs (1)
26-36: Use a keyed relic index for both sync passes.Both methods repeatedly scan
relicswithFirstOrDefault. A shared dictionary keyed by(EraName, NameInEra)(case-insensitive) will reduce lookup overhead and centralize matching logic.Also applies to: 63-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AllFilteredGenerator/Relic.cs` around lines 26 - 36, Replace repeated FirstOrDefault scans over the relics list with a single case-insensitive lookup dictionary built once at the start of SyncRelicsWithParts: create a Dictionary keyed by a combined key (e.g. $"{EraName}|{NameInEra}".ToLowerInvariant()) from the relics collection and use it to find matches for part.DroppingRelics instead of relics.FirstOrDefault; apply the same dictionary-based lookup to the other sync pass referenced around lines 63-68 so both SyncRelicsWithParts and the counterpart use the shared keyed index for matchingRelic lookups to centralize matching logic and improve performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AllFilteredGenerator/JsonExtensions.cs`:
- Around line 81-92: The threshold check is currently done against a rounded int
(chancePercent) which loses precision; update the logic in JsonExtensions.cs
(the block that computes chancePercent from chance.Value and checks
rarity.Equals("Uncommon", ...)) to perform the > 11 comparison against the
unrounded value instead—i.e., compute the multiplier (100 when chance < 1,
otherwise 1), test if chance.Value * multiplier > 11 for the "Uncommon" branch,
and only then round/assign chancePercent for downstream use so values like 11.2
are correctly handled.
- Around line 39-67: The parsing in GetIntProperty and GetDoubleProperty uses
culture-sensitive TryParse overloads; update both to use the IFormatProvider
overloads with CultureInfo.InvariantCulture (and appropriate NumberStyles:
NumberStyles.Integer for GetIntProperty and NumberStyles.Float |
NumberStyles.AllowThousands for GetDoubleProperty) when calling int.TryParse and
double.TryParse on propNode.ToString(), so JSON numeric strings parse
deterministically regardless of host locale.
In `@AllFilteredGenerator/Program.cs`:
- Around line 32-36: The error message in the Program.cs file's missing-file
check prints the wrong variable; update the Console.WriteLine inside the if
(!File.Exists(allJsonPath)) block to reference allJsonPath (and include a space
after the colon) instead of savePath so the user sees the actual missing file
path; locate the if block that checks File.Exists(allJsonPath) and change the
string concatenation to use allJsonPath in the Console.WriteLine call.
In `@AllFilteredGenerator/Properties/PublishProfiles/linux.pubxml`:
- Around line 7-10: Update the publish profile so its TargetFramework matches
the project target (change the TargetFramework element in the linux.pubxml from
net8.0 to net10.0); locate the linux.pubxml publish profile and modify the
<TargetFramework> element (and any related PublishDir paths that embed net8.0,
e.g., the <PublishDir> value) so they consistently reference net10.0 to avoid
publish/runtime mismatches when building AllFilteredGenerator.
In `@AllFilteredGenerator/Properties/PublishProfiles/windows.pubxml`:
- Around line 7-10: The windows publish profile's TargetFramework element
currently reads net8.0 but the project targets net10.0; update the
<TargetFramework> value in
AllFilteredGenerator/Properties/PublishProfiles/windows.pubxml (the
TargetFramework XML element in that file) to net10.0 so the publish profile
matches the project target, and verify related publish paths
(PublishDir/PublishProtocol/_TargetId) remain correct for the win-x64 publish.
---
Nitpick comments:
In `@AllFilteredGenerator/IndependentPrimeComponent.cs`:
- Around line 21-28: The method IndependentPrimeComponent.TryParse always
returns a non-null IndependentPrimeComponent, so rename TryParse to Parse
(change method name to Parse in the IndependentPrimeComponent class) and update
all call sites (notably the usage in PrimeEquipment.cs) to call
IndependentPrimeComponent.Parse; ensure the method signature and any XML
comments are updated accordingly and run a solution-wide rename to catch any
other references.
In `@AllFilteredGenerator/ItemRelicInfo.cs`:
- Around line 67-71: In the loop inside ItemRelicInfo (remove the redundant else
{ continue; } block): delete the else branch so the loop simply proceeds
naturally when the if condition fails; keep the code path for the if branch
intact and ensure subsequent statements remain at the same level (no further
changes to method or variable names required).
In `@AllFilteredGenerator/PrimeComponent.cs`:
- Around line 25-62: TryParse currently always constructs and returns a
PrimeComponent despite its nullable signature; either remove the nullable or
return null when critical ducat data is absent—preferably implement Option 2:
detect missing ducat/primeSellingPrice (when parsedDucats and
parsedPrimeSellingPrice are both null), log the error (errors.Add) and return
null instead of creating a PrimeComponent; update the TryParse method so the
branch where you set savedDucatCount to 0 and call errors.Add yields an
immediate return null, leaving PrimeComponent(...) construction only in the
success path.
In `@AllFilteredGenerator/PrimeEquipment.cs`:
- Around line 29-45: In SyncIndependentParts, avoid repeated linear scans by
prebuilding a case-insensitive lookup (e.g., Dictionary<string, PrimeEquipment>
using StringComparer.InvariantCultureIgnoreCase) mapping prime.Name to the
PrimeEquipment, then replace primes.FirstOrDefault(...) with a
dictionary.TryGetValue on that map; when a match is found preserve the existing
logic (set part.Vaulted = matchingItem.Vaulted and prime.Vaulted |=
matchingItem.Vaulted) and when not found add the error as before.
In `@AllFilteredGenerator/Program.cs`:
- Around line 41-44: The code currently throws an ArgumentException when the
parsed JSON is null or not an array (in the block checking json is null or
json.GetValueKind() is not JsonValueKind.Array), which is inconsistent with
other error handling that calls Environment.Exit(1) for missing args/file;
change this to use the same exit pattern: log or write a clear error message
(matching other messages for missing args/file) and call Environment.Exit(1)
instead of throwing, ensuring the message identifies the file/condition and
locating the change in the JSON validation block (the if checking json is null
|| json.GetValueKind() is not JsonValueKind.Array).
- Around line 177-189: Remove the dead commented-out code inside the else block
that references elemObj.GetStringProperty("category"), name.Equals("Forma"),
ItemRelicInfo.TryParseDrops and formaRelicInfo — either delete the entire empty
else block or replace it with a brief explanatory comment if you intentionally
preserved it for historical reasons; ensure no commented legacy logic remains in
Program.cs so only active code or a clear rationale comment remains.
In `@AllFilteredGenerator/Relic.cs`:
- Around line 26-36: Replace repeated FirstOrDefault scans over the relics list
with a single case-insensitive lookup dictionary built once at the start of
SyncRelicsWithParts: create a Dictionary keyed by a combined key (e.g.
$"{EraName}|{NameInEra}".ToLowerInvariant()) from the relics collection and use
it to find matches for part.DroppingRelics instead of relics.FirstOrDefault;
apply the same dictionary-based lookup to the other sync pass referenced around
lines 63-68 so both SyncRelicsWithParts and the counterpart use the shared keyed
index for matchingRelic lookups to centralize matching logic and improve
performance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90860bac-4f4f-439f-a992-ff797673f04b
📒 Files selected for processing (11)
AllFilteredGenerator/AllFilteredGenerator.csprojAllFilteredGenerator/IndependentPrimeComponent.csAllFilteredGenerator/ItemRelicInfo.csAllFilteredGenerator/JsonExtensions.csAllFilteredGenerator/PrimeComponent.csAllFilteredGenerator/PrimeEquipment.csAllFilteredGenerator/Program.csAllFilteredGenerator/Properties/PublishProfiles/linux.pubxmlAllFilteredGenerator/Properties/PublishProfiles/windows.pubxmlAllFilteredGenerator/Relic.csWFInfo.sln
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
1 participant
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.
What did you fix?
Ported code for generating All_Filtered.json from google apps script to C# cross-platform CLI tool.
Enables transition to the API host performing conversion, instead of having to periodically fetch from google drive.
(Does not do the same for Warframe.market data)
Reproduction steps
Building:
AllFilteredGeneratorfolderdotnet publishRunning:
All.jsonfile of warframe-items repo (or file dump of corresponding web API with english language specified. I think it should be able to handle either)Evidence/screenshot/link to line
Compare with live version from https://api.warframestat.us/wfinfo/filtered_items, Only difference should be time, errors in live version or updates yet to be processed by live version.
Considerations
Summary by CodeRabbit
New Features
Chores