Simplify item ID resolution to drop module-prefixed lookup#328
Open
Travja wants to merge 2 commits into
Open
Conversation
DivinityProvider previously namespaced item IDs by their owning module (e.g. "custom_items:foobar") when resolving items by ItemStack or ID string. Since item IDs are unique across modules in practice, this indirection added complexity without a real need; getItem()/getID() now resolve by plain item ID. Note for reviewers: this also drops the DivinityProviderTest cases that covered the namespaced-lookup behavior (getItem_namespacedIdIncludesModule, getItem_itemStackUsesStoredModule), since that behavior no longer exists.
getItem(String) already parses the legacy "module:id" namespaced form unchanged, so external references captured via the old getID() format still resolve correctly. isCustomItemOfId did not have an equivalent fallback: it moved straight to plain-ID equality, so any caller still passing a namespaced id here (matching this method's previous contract) would always get false after the simplification. Fall back to the old module+split check when plain equality fails and the id looks namespaced, so existing callers aren't silently broken.
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
2 participants
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.
Split out of #320 (piece 3/12, independent).
DivinityProviderpreviously namespaced item IDs by their owning module (e.g.custom_items:foobar) when resolving items byItemStackor ID string. Since item IDs are unique across modules in practice, this indirection added complexity without a real need;getItem()/getID()now resolve by plain item ID.Note for reviewers: this also drops the
DivinityProviderTestcases that covered the namespaced-lookup behavior (getItem_namespacedIdIncludesModule,getItem_itemStackUsesStoredModule), since that behavior no longer exists. Please confirm no code outside this diff still depends on module-prefixed IDs before merging.Backward compatibility
getItem(String)already parses the legacy"module:id"namespaced form unchanged (untouched by this diff), so any stored/external reference captured via the oldgetID()format still resolves correctly.isCustomItemOfIddidn't have an equivalent fallback — added one so it still recognizes the namespaced form when plain-ID equality doesn't match, instead of always returning false for legacy callers.