Refactor Item API: namespace and builder cleanup#82
Open
k073l wants to merge 8 commits into
Open
Conversation
# Conflicts: # S1API/Items/ClothingItemInstance.cs # S1API/Items/QualityItemInstance.cs
📝 WalkthroughWalkthroughAdds namespaced item APIs (Storable, Additive, Buildable, Clothing, Quality): new wrapper types, fluent builders, creator helpers, registry wiring, station-item caching and one-time warnings; marks legacy root-level types obsolete and adds ID/argument validation and ItemManager lookup migration. ChangesAdditive Item API Reorganization
Buildable Item API Reorganization
Clothing Item API Reorganization
Quality Item API Reorganization
Storable Item API Reorganization
Root Items API Consolidation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
S1API/Items/ItemCreator.cs (1)
110-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the legacy
CreateItem(... requiresLevelToPurchase ...)overload (and translate it torequiredRank).
S1API/Items/ItemCreator.cs’s obsoleteCreateItemoverload no longer includesrequiresLevelToPurchase, and there’s no backward-compat shim overload present, so precompiled callers will fail to bind to the old method token. Also,RequiresLevelToPurchaseis currently driven byrequiredRankviaStorableItemDefinitionBuilder.WithRequiredRank(null=> false, non-null => true), so the compatibility overload must maprequiresLevelToPurchaseintorequiredRankrather than ignoring it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Items/ItemCreator.cs` around lines 110 - 121, Add a backward-compatible overload for CreateItem that accepts the legacy bool requiresLevelToPurchase and forwards to the existing CreateItem by translating that bool into the nullable FullRank requiredRank used by the new API; implement CreateItem(..., bool requiresLevelToPurchase, FullRank? requiredRank = null, ...) so it computes translatedRequiredRank = requiresLevelToPurchase ? (requiredRank ?? <legacy-default-FullRank>) : null and calls the existing CreateItem(id, name, description, category, stackLimit, basePurchasePrice, resellMultiplier, legalStatus, translatedRequiredRank, icon, equippable); ensure you reference the ItemCreator.CreateItem overload and use StorableItemDefinitionBuilder.WithRequiredRank semantics (null => no requirement, non-null => requires level).
🧹 Nitpick comments (3)
S1API/Items/Quality/QualityItemDefinitionBuilder.cs (1)
87-91: ⚡ Quick winWrap the method parameter in
CreateWrapperinstead of builder state.Line [90] ignores the
definitionparameter and usesQualityDefinition. Using the passed argument avoids accidental mismatch if base build flow changes.Proposed fix
protected override Storable.StorableItemDefinition CreateWrapper( S1ItemFramework.StorableItemDefinition definition) { - return new QualityItemDefinition(QualityDefinition); + return new QualityItemDefinition( + CrossType.As<S1ItemFramework.QualityItemDefinition>(definition)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Items/Quality/QualityItemDefinitionBuilder.cs` around lines 87 - 91, The CreateWrapper method currently ignores its parameter and uses the builder's QualityDefinition field; change it to use the incoming S1ItemFramework.StorableItemDefinition parameter (`definition`) when constructing the wrapper so the method respects the value provided by the base build flow (i.e., call new QualityItemDefinition(definition) inside CreateWrapper instead of referencing QualityDefinition).S1API/Items/Additive/AdditiveDefinitionBuilder.cs (1)
129-133: ⚡ Quick winUse the
definitionargument inCreateWrapper.This override currently ignores its input and wraps the field-backed instance instead. Wrapping the provided argument keeps behavior aligned with the base contract.
Suggested diff
protected override Storable.StorableItemDefinition CreateWrapper( S1ItemFramework.StorableItemDefinition definition) { - return new AdditiveDefinition(AdditiveDefinition); + var additiveDefinition = CrossType.As<S1ItemFramework.AdditiveDefinition>(definition); + return new AdditiveDefinition(additiveDefinition); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Items/Additive/AdditiveDefinitionBuilder.cs` around lines 129 - 133, The CreateWrapper override currently ignores its input parameter and uses the field-backed AdditiveDefinition; update CreateWrapper to construct and return the wrapper using the provided parameter (i.e. call the AdditiveDefinition constructor with the method parameter `definition` rather than the field) so the override honors the base contract and wraps the passed-in S1ItemFramework.StorableItemDefinition.S1API/Items/ItemManager.cs (1)
78-124: ⚡ Quick winUse
GetDefinitioninternally so non-obsolete APIs stop routing through legacy wrappers.You added the new resolver here, but Line 211 and Line 258 still call
GetItemDefinition. That keepsUnregisterItem/GetAllItemDefinitionson the deprecated path and returns legacy wrapper types from a non-obsolete API surface.Targeted follow-up diff
- ItemDefinition definition = GetItemDefinition(itemID); + ItemDefinition? definition = GetDefinition(itemID); if (definition == null) { return false; } RemoveFromRuntimeCleanupQueue(definition.S1ItemDefinition, definition.ID); S1Registry.Instance.RemoveFromRegistry(definition.S1ItemDefinition); - return GetItemDefinition(itemID) == null; + return GetDefinition(itemID) == null; ... - var wrappedItem = GetItemDefinition(itemId); + var wrappedItem = GetDefinition(itemId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Items/ItemManager.cs` around lines 78 - 124, GetDefinition was introduced to return the new item-definition types but some callers (specifically UnregisterItem and GetAllItemDefinitions) still call the legacy GetItemDefinition, causing deprecated wrappers to be returned; update those callers to call GetDefinition instead so UnregisterItem and GetAllItemDefinitions (and any other places at the noted call sites) use the new resolver and return the non-obsolete types, ensuring you replace uses of GetItemDefinition with GetDefinition in the methods that currently route through the legacy path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@S1API/Items/Additive/AdditiveDefinitionBuilder.cs`:
- Around line 84-95: In WithEffects (AdditiveDefinitionBuilder.WithEffects)
clamp the instantGrowth argument to the documented 0..1 range before persisting
it: compute a clampedInstant = MathF.Clamp(instantGrowth, 0f, 1f) (or Math.Clamp
if preferred) and pass clampedInstant into AutoPropertySetter.TrySet for
AdditiveDefinition.InstantGrowth; leave the existing warning/log behavior
unchanged and only change the value being set.
In `@S1API/Items/Additive/AdditiveItemCreator.cs`:
- Around line 34-39: In CloneFrom(string sourceItemId) validate sourceItemId for
null or whitespace before calling S1Registry.GetItem: if
string.IsNullOrWhiteSpace(sourceItemId) throw new
ArgumentException("sourceItemId must be provided", nameof(sourceItemId)); then
proceed to call S1Registry.GetItem and keep the existing not-found
ArgumentException; this ensures CloneFrom returns a deterministic argument error
for bad input and still throws the existing error when the registry lookup
fails.
In `@S1API/Items/Buildable/BuildableItemCreator.cs`:
- Around line 87-88: Update the example to use the new namespaced lookup API:
replace the call to ItemManager.GetItemDefinition(...) with
ItemManager.GetDefinition(...), so the sample that shows var originalRack =
ItemManager.GetItemDefinition("StorageRack-1x0.5") as BuildableItemDefinition
uses ItemManager.GetDefinition("StorageRack-1x0.5") instead; ensure the rest of
the example still references BuildableItemCreator.CloneFrom and casts to
S1API.Items.Buildable.BuildableItemDefinition (or uses a safe null check) to
avoid failed casts/nulls.
In `@S1API/Items/Buildable/BuildableItemDefinitionBuilder.cs`:
- Around line 59-61: The WithBuildSound method is using the legacy
Items.BuildSoundType; change its parameter to the new namespaced enum
S1API.Items.Buildable.BuildSoundType in the
BuildableItemDefinitionBuilder.WithBuildSound signature and update the cast
inside the method to cast from S1API.Items.Buildable.BuildSoundType to
S1ItemFramework.BuildableItemDefinition.EBuildSoundType (keeping
BuildableDefinition.BuildSoundType assignment as-is) so the new builder API no
longer leaks the deprecated root enum.
In `@S1API/Items/BuildableItemDefinitionBuilder.cs`:
- Line 194: In BuildableItemDefinitionBuilder replace the nullable ID check to
reject whitespace-only strings: change the guard that checks _definition.ID from
string.IsNullOrEmpty(...) to string.IsNullOrWhiteSpace(...); also ensure the
exception type reflects invalid argument (use ArgumentException with
nameof(_definition.ID)) so whitespace-only IDs are rejected at the same check
point.
In `@S1API/Items/Clothing/ClothingItemCreator.cs`:
- Around line 60-67: In CloneFrom (ClothingItemCreator.cs) add a guard at the
start to validate sourceItemId (use string.IsNullOrWhiteSpace) and fail early:
if the input is null/empty/whitespace, log a clear error via Logger.Error
mentioning the invalid sourceItemId and return null before calling
S1.Registry.GetItem; keep the existing behavior for registry-miss unchanged.
In `@S1API/Items/Clothing/ClothingItemDefinitionBuilder.cs`:
- Around line 156-169: The public API for WithBlockedSlots currently accepts
Clothing.ClothingColor[] but immediately casts to S1Clothing.EClothingSlot;
change the parameter to accept the correct slot type (e.g., ClothingSlot[] or
the internal S1Clothing.EClothingSlot array type you want exposed) so callers
use the intended slot enum, and update both branches that assign
ClothingDefinition.SlotsToBlock (the IL2CPPMELON branch creating
Il2CppCollections.List<S1Clothing.EClothingSlot> and the non-IL2CPPMELON branch
creating List<S1Clothing.EClothingSlot>) to add items from the new parameter
without mismatched casts; ensure method signature WithBlockedSlots(...) and any
casting code that adds to ClothingDefinition.SlotsToBlock are updated
consistently.
In `@S1API/Items/ClothingItemDefinitionBuilder.cs`:
- Line 242: The current validation in ClothingItemDefinitionBuilder checks only
for null or empty ID using string.IsNullOrEmpty(_definition.ID); update this to
reject whitespace-only IDs by using string.IsNullOrWhiteSpace and throw an
ArgumentException (not ArgumentNullException) when the ID is
null/empty/whitespace; locate the check around the _definition.ID validation and
replace the condition and exception accordingly so invalid whitespace IDs are
rejected.
In `@S1API/Items/Quality/QualityItemCreator.cs`:
- Around line 35-41: In CloneFrom, validate the sourceItemId parameter for null
or whitespace before calling S1Registry.GetItem: check
string.IsNullOrWhiteSpace(sourceItemId) and throw an ArgumentException (or
ArgumentNullException) referencing nameof(sourceItemId) if invalid, then proceed
to call S1Registry.GetItem(sourceItemId) and keep the existing not-found
exception behavior for when the registry returns null.
In `@S1API/Items/QualityItemDefinitionBuilder.cs`:
- Line 278: The current check in QualityItemDefinitionBuilder that does "if
(string.IsNullOrEmpty(_definition.ID)) throw new
ArgumentNullException(nameof(_definition.ID));" conflates null and empty; update
the validation in the builder (where _definition.ID is validated) to first test
for null and throw ArgumentNullException(nameof(_definition.ID)) and then test
for empty (e.g., _definition.ID == string.Empty or _definition.ID.Length == 0)
and throw new ArgumentException("ID cannot be empty", nameof(_definition.ID));
so callers can distinguish null vs. present-but-empty IDs.
In `@S1API/Items/Storable/StorableItemDefinitionBuilder.cs`:
- Around line 373-394: The Build() method in StorableItemDefinitionBuilder
validates Definition.ID but throws ArgumentNullException with Definition.ID
(which may be null/empty); change this to throw an InvalidOperationException
instead with a clear message (e.g., "Cannot build item definition: ID is
required.") while keeping the existing Logger.Error call; update the exception
construction in Build() (and any similar validation in the class) to use
InvalidOperationException and include a descriptive message rather than passing
Definition.ID as the parameter name.
---
Outside diff comments:
In `@S1API/Items/ItemCreator.cs`:
- Around line 110-121: Add a backward-compatible overload for CreateItem that
accepts the legacy bool requiresLevelToPurchase and forwards to the existing
CreateItem by translating that bool into the nullable FullRank requiredRank used
by the new API; implement CreateItem(..., bool requiresLevelToPurchase,
FullRank? requiredRank = null, ...) so it computes translatedRequiredRank =
requiresLevelToPurchase ? (requiredRank ?? <legacy-default-FullRank>) : null and
calls the existing CreateItem(id, name, description, category, stackLimit,
basePurchasePrice, resellMultiplier, legalStatus, translatedRequiredRank, icon,
equippable); ensure you reference the ItemCreator.CreateItem overload and use
StorableItemDefinitionBuilder.WithRequiredRank semantics (null => no
requirement, non-null => requires level).
---
Nitpick comments:
In `@S1API/Items/Additive/AdditiveDefinitionBuilder.cs`:
- Around line 129-133: The CreateWrapper override currently ignores its input
parameter and uses the field-backed AdditiveDefinition; update CreateWrapper to
construct and return the wrapper using the provided parameter (i.e. call the
AdditiveDefinition constructor with the method parameter `definition` rather
than the field) so the override honors the base contract and wraps the passed-in
S1ItemFramework.StorableItemDefinition.
In `@S1API/Items/ItemManager.cs`:
- Around line 78-124: GetDefinition was introduced to return the new
item-definition types but some callers (specifically UnregisterItem and
GetAllItemDefinitions) still call the legacy GetItemDefinition, causing
deprecated wrappers to be returned; update those callers to call GetDefinition
instead so UnregisterItem and GetAllItemDefinitions (and any other places at the
noted call sites) use the new resolver and return the non-obsolete types,
ensuring you replace uses of GetItemDefinition with GetDefinition in the methods
that currently route through the legacy path.
In `@S1API/Items/Quality/QualityItemDefinitionBuilder.cs`:
- Around line 87-91: The CreateWrapper method currently ignores its parameter
and uses the builder's QualityDefinition field; change it to use the incoming
S1ItemFramework.StorableItemDefinition parameter (`definition`) when
constructing the wrapper so the method respects the value provided by the base
build flow (i.e., call new QualityItemDefinition(definition) inside
CreateWrapper instead of referencing QualityDefinition).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d1f6f12-da60-4ca7-b523-f590521b1174
📒 Files selected for processing (41)
S1API/Items/Additive/AdditiveDefinition.csS1API/Items/Additive/AdditiveDefinitionBuilder.csS1API/Items/Additive/AdditiveItemCreator.csS1API/Items/AdditiveDefinition.csS1API/Items/AdditiveDefinitionBuilder.csS1API/Items/AdditiveItemCreator.csS1API/Items/Buildable/BuildableItemCreator.csS1API/Items/Buildable/BuildableItemDefinition.csS1API/Items/Buildable/BuildableItemDefinitionBuilder.csS1API/Items/BuildableItemCreator.csS1API/Items/BuildableItemDefinition.csS1API/Items/BuildableItemDefinitionBuilder.csS1API/Items/Clothing/ClothingApplicationType.csS1API/Items/Clothing/ClothingColor.csS1API/Items/Clothing/ClothingItemCreator.csS1API/Items/Clothing/ClothingItemDefinition.csS1API/Items/Clothing/ClothingItemDefinitionBuilder.csS1API/Items/Clothing/ClothingItemInstance.csS1API/Items/Clothing/ClothingSlot.csS1API/Items/ClothingApplicationType.csS1API/Items/ClothingColor.csS1API/Items/ClothingItemCreator.csS1API/Items/ClothingItemDefinition.csS1API/Items/ClothingItemDefinitionBuilder.csS1API/Items/ClothingItemInstance.csS1API/Items/ClothingSlot.csS1API/Items/ItemCreator.csS1API/Items/ItemManager.csS1API/Items/Quality/QualityItemCreator.csS1API/Items/Quality/QualityItemDefinition.csS1API/Items/Quality/QualityItemDefinitionBuilder.csS1API/Items/Quality/QualityItemInstance.csS1API/Items/QualityItemCreator.csS1API/Items/QualityItemDefinition.csS1API/Items/QualityItemDefinitionBuilder.csS1API/Items/QualityItemInstance.csS1API/Items/Storable/ItemCreator.csS1API/Items/Storable/StorableItemDefinition.csS1API/Items/Storable/StorableItemDefinitionBuilder.csS1API/Items/StorableItemDefinition.csS1API/Items/StorableItemDefinitionBuilder.cs
Comment on lines
+156
to
+169
| public ClothingItemDefinitionBuilder WithBlockedSlots(params Clothing.ClothingColor[] slots) | ||
| { | ||
| #if (IL2CPPMELON) | ||
| ClothingDefinition.SlotsToBlock = new Il2CppCollections.List<S1Clothing.EClothingSlot>(); | ||
| foreach (var slot in slots) | ||
| { | ||
| ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot); | ||
| } | ||
| #else | ||
| ClothingDefinition.SlotsToBlock = new List<S1Clothing.EClothingSlot>(); | ||
| foreach (var slot in slots) | ||
| { | ||
| ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot); | ||
| } |
There was a problem hiding this comment.
Use ClothingSlot in WithBlockedSlots API.
Line 156 takes Clothing.ClothingColor[], but Lines 162/168 cast to EClothingSlot. This exposes the wrong public contract and makes normal slot usage awkward/error-prone.
Proposed fix
- public ClothingItemDefinitionBuilder WithBlockedSlots(params Clothing.ClothingColor[] slots)
+ public ClothingItemDefinitionBuilder WithBlockedSlots(params Clothing.ClothingSlot[] slots)
{
`#if` (IL2CPPMELON)
ClothingDefinition.SlotsToBlock = new Il2CppCollections.List<S1Clothing.EClothingSlot>();
foreach (var slot in slots)
{
ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot);
}
`#else`
ClothingDefinition.SlotsToBlock = new List<S1Clothing.EClothingSlot>();
foreach (var slot in slots)
{
ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot);
}
`#endif`
return this;
}📝 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.
Suggested change
| public ClothingItemDefinitionBuilder WithBlockedSlots(params Clothing.ClothingColor[] slots) | |
| { | |
| #if (IL2CPPMELON) | |
| ClothingDefinition.SlotsToBlock = new Il2CppCollections.List<S1Clothing.EClothingSlot>(); | |
| foreach (var slot in slots) | |
| { | |
| ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot); | |
| } | |
| #else | |
| ClothingDefinition.SlotsToBlock = new List<S1Clothing.EClothingSlot>(); | |
| foreach (var slot in slots) | |
| { | |
| ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot); | |
| } | |
| public ClothingItemDefinitionBuilder WithBlockedSlots(params Clothing.ClothingSlot[] slots) | |
| { | |
| `#if` (IL2CPPMELON) | |
| ClothingDefinition.SlotsToBlock = new Il2CppCollections.List<S1Clothing.EClothingSlot>(); | |
| foreach (var slot in slots) | |
| { | |
| ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot); | |
| } | |
| `#else` | |
| ClothingDefinition.SlotsToBlock = new List<S1Clothing.EClothingSlot>(); | |
| foreach (var slot in slots) | |
| { | |
| ClothingDefinition.SlotsToBlock.Add((S1Clothing.EClothingSlot)slot); | |
| } | |
| `#endif` | |
| return this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S1API/Items/Clothing/ClothingItemDefinitionBuilder.cs` around lines 156 -
169, The public API for WithBlockedSlots currently accepts
Clothing.ClothingColor[] but immediately casts to S1Clothing.EClothingSlot;
change the parameter to accept the correct slot type (e.g., ClothingSlot[] or
the internal S1Clothing.EClothingSlot array type you want exposed) so callers
use the intended slot enum, and update both branches that assign
ClothingDefinition.SlotsToBlock (the IL2CPPMELON branch creating
Il2CppCollections.List<S1Clothing.EClothingSlot> and the non-IL2CPPMELON branch
creating List<S1Clothing.EClothingSlot>) to add items from the new parameter
without mismatched casts; ensure method signature WithBlockedSlots(...) and any
casting code that adds to ClothingDefinition.SlotsToBlock are updated
consistently.
| /// <returns>A wrapper around the created storable item definition.</returns> | ||
| public QualityItemDefinition Build() | ||
| { | ||
| if (string.IsNullOrEmpty(_definition.ID)) throw new ArgumentNullException(nameof(_definition.ID)); |
There was a problem hiding this comment.
Use the correct exception type for empty IDs.
Line [278] throws ArgumentNullException for both null and empty. Empty should throw ArgumentException so callers can distinguish invalid-but-present values.
Proposed fix
- if (string.IsNullOrEmpty(_definition.ID)) throw new ArgumentNullException(nameof(_definition.ID));
+ if (_definition.ID == null)
+ throw new ArgumentNullException(nameof(_definition.ID));
+ if (_definition.ID.Length == 0)
+ throw new ArgumentException("Item ID cannot be empty.", nameof(_definition.ID));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S1API/Items/QualityItemDefinitionBuilder.cs` at line 278, The current check
in QualityItemDefinitionBuilder that does "if
(string.IsNullOrEmpty(_definition.ID)) throw new
ArgumentNullException(nameof(_definition.ID));" conflates null and empty; update
the validation in the builder (where _definition.ID is validated) to first test
for null and throw ArgumentNullException(nameof(_definition.ID)) and then test
for empty (e.g., _definition.ID == string.Empty or _definition.ID.Length == 0)
and throw new ArgumentException("ID cannot be empty", nameof(_definition.ID));
so callers can distinguish null vs. present-but-empty IDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@S1API/Items/ItemManager.cs`:
- Line 258: GetAllItemDefinitions() currently routes through GetDefinition which
returns namespaced wrapper types and breaks callers that expect legacy root
wrapper types; restore the legacy contract by having GetAllItemDefinitions()
call the legacy wrapper method GetItemDefinition (or iterate and call
GetItemDefinition for each id) so its results remain the old root wrapper type,
or alternatively mark GetAllItemDefinitions() [Obsolete] and introduce a new
method (e.g., GetAllNamespacedItemDefinitions) that delegates to GetDefinition;
update callers/tests accordingly and ensure method signatures and return types
match the legacy root wrapper type to avoid runtime downcast failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 321a214a-d1b0-40f9-87eb-0048130dfa40
📒 Files selected for processing (13)
S1API/Items/Additive/AdditiveDefinitionBuilder.csS1API/Items/Additive/AdditiveItemCreator.csS1API/Items/Buildable/BuildableItemCreator.csS1API/Items/Buildable/BuildableItemDefinitionBuilder.csS1API/Items/BuildableItemDefinitionBuilder.csS1API/Items/Clothing/ClothingItemCreator.csS1API/Items/ClothingItemDefinitionBuilder.csS1API/Items/ItemManager.csS1API/Items/Quality/QualityItemCreator.csS1API/Items/QualityItemDefinitionBuilder.csS1API/Items/Storable/ItemCreator.csS1API/Items/Storable/StorableItemDefinitionBuilder.csS1API/Items/StorableItemDefinitionBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- S1API/Items/StorableItemDefinitionBuilder.cs
- S1API/Items/Buildable/BuildableItemDefinitionBuilder.cs
- S1API/Items/Quality/QualityItemCreator.cs
- S1API/Items/Additive/AdditiveItemCreator.cs
- S1API/Items/Clothing/ClothingItemCreator.cs
- S1API/Items/Storable/ItemCreator.cs
- S1API/Items/Buildable/BuildableItemCreator.cs
- S1API/Items/Storable/StorableItemDefinitionBuilder.cs
|
|
||
| // Use GetItemDefinition to properly wrap the item with the correct type | ||
| var wrappedItem = GetItemDefinition(itemId); | ||
| var wrappedItem = GetDefinition(itemId); |
There was a problem hiding this comment.
Preserve GetAllItemDefinitions() legacy wrapper contract.
GetAllItemDefinitions() is still non-obsolete, but it now routes through GetDefinition, which returns namespaced wrapper types. Existing consumers that downcast results to legacy root wrapper types can break at runtime. Keep this method on legacy wrapping (GetItemDefinition) or mark it obsolete and introduce a new method for namespaced wrappers.
Proposed minimal compatibility fix
- // Use GetItemDefinition to properly wrap the item with the correct type
- var wrappedItem = GetDefinition(itemId);
+ // Keep legacy contract for this non-obsolete API.
+#pragma warning disable CS0618
+ var wrappedItem = GetItemDefinition(itemId);
+#pragma warning restore CS0618🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@S1API/Items/ItemManager.cs` at line 258, GetAllItemDefinitions() currently
routes through GetDefinition which returns namespaced wrapper types and breaks
callers that expect legacy root wrapper types; restore the legacy contract by
having GetAllItemDefinitions() call the legacy wrapper method GetItemDefinition
(or iterate and call GetItemDefinition for each id) so its results remain the
old root wrapper type, or alternatively mark GetAllItemDefinitions() [Obsolete]
and introduce a new method (e.g., GetAllNamespacedItemDefinitions) that
delegates to GetDefinition; update callers/tests accordingly and ensure method
signatures and return types match the legacy root wrapper type to avoid runtime
downcast failures.
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.
Copied existing item classes to their logical namespaces - e.g.
QualityItemInstance,QualityItemDefinitionandQualityItemDefinitionBuilderintoQualitynamespace.Refactored builders -
StorableItemDefinitionBuilderBaseuses CRTP to preserve fluency of the builder, while allowing for subclassing. This approach reduces code duplication - child definitions ofStorableItemDefinitionno longer have to define their own.WithBasicInfo(...)and other commmon methods in their specific builders.Original files are preserved, but marked as
Obsoletefor compatibility reasons and can be removed in the future release.I also obsoleted
GetItemDefinitioninItemManager, though I'm not sure if that's the best course of action. Some of the types returned from it are marked as Obsolete by this PR, so I figured it's better to make a new method that returns the new ones. Let me know if there's a better way of handling this.