Repository navigation
loader: pass tags through to mcData - #457
VasilisDragon wants to merge 2 commits into
Conversation
lib/loader.js returns a fixed object, so the tags key emitted by dataPaths-driven data.js getters never reached mcData even once tags data exists in the submodule (minecraft-data#1080). Add the passthrough plus typings, docs, and a per-version shape contract test. With the current submodule pin no version has tags data, so mcData.tags is undefined everywhere: this is inert now and activates on the first submodule bump after minecraft-data#1080 and its schema land. Resolves PrismarineJS#315. Loader half of the plan discussed in minecraft-data#412.
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The optional passthrough and structural type are appropriate: they preserve the source registry map without inventing an ID index, and remain absent when data is absent. One integration correction is needed before landing the public examples: the current chosen data/schema in minecraft-data #1308/#1194 use namespaced registry keys.
The inline change is about that concrete contract, not a request to wait for the later dig-time redesign. No new full loader suite was run in this publication pass.
Skills used: prismarine-review checked the current revision and discussion; prismarine-protocol-data-review checked the version-selected producer/consumer contract; prismarine-architecture-review checked package ownership and integration scope.
| Example: | ||
|
|
||
| ```js | ||
| console.log(mcData.tags.block['minecraft:mineable/pickaxe']) |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The active tags contract in minecraft-data #1308/#1194 uses minecraft:block, not block. With that dataset, this documented expression reads an undefined registry and throws. Please use mcData.tags['minecraft:block']['minecraft:mineable/pickaxe'] when tags are present, and update the type comment and typings/test-typings.ts example too. The passthrough itself already supports the namespaced keys, so no runtime remapping is needed.
Skills used: prismarine-review checked the current revision and discussion; prismarine-protocol-data-review checked the version-selected producer/consumer contract; prismarine-architecture-review checked package ownership and integration scope.
There was a problem hiding this comment.
Updated the API example, type comment and typing example to use minecraft:block. The examples also handle versions without tags data. The loader remains a passthrough.
rom1504
left a comment
rom1504
left a comment
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
One additional testing finding: the new tags loop never detects loss of the passthrough it is meant to protect. I inspected the candidate npm test/Mocha route and ran the focused loader control described inline; I did not rerun the complete wrapper suite.
Skills used: prismarine-review supplied the meaningful-assertion gate; prismarine-protocol-data-review traced source data versus loader output; prismarine-architecture-review kept the repair in the existing loader suite.
| const data = mcData(type + '_' + version.minecraftVersion) | ||
| if (!data || data.tags === undefined) return | ||
| withTags++ | ||
| for (const registry in data.tags) { |
There was a problem hiding this comment.
Astra agent review — AI-generated, not manually written by the maintainer.
The new regression can pass with the entire tags: mcData.tags fix removed: this guard then returns for every version, and the after hook only logs the zero count. The PR correctly notes that the pinned dataset has no tags; even after a tags-bearing data bump, dropping the loader passthrough would still silently skip every assertion.
Please add an always-executed positive case in the existing loader test suite: pass a small namespaced tags map through the real lib/loader and assert that it is preserved, alongside the absent-data case. Keep this version loop as supplementary coverage (or compare its expectation against the source data rather than deciding applicability from the output under test). A focused control with this head's real loader/indexers passed the current shape condition both with and without the passthrough, while the explicit preservation assertion failed when the passthrough was removed. This is a test-quality issue, not a claim that the current loader fix is wrong.
Skills used: prismarine-review supplied the meaningful-assertion gate; prismarine-protocol-data-review traced source data versus loader output; prismarine-architecture-review kept the repair in the existing loader suite.
There was a problem hiding this comment.
Added two direct loader cases for a namespaced tags map and absent data. Removing the passthrough now fails the positive case, while the absent-data case still passes. The version loop remains supplementary; the pinned data has no tags-bearing versions. The full test chain passes on the revised branch and after integrating current master.
The loader currently drops
tagseven when the source provides them. This passes the optional map through unchanged asmcData.tags, adds its TypeScript declaration, and documents namespaced access such asmcData.tags?.['minecraft:block']?.['minecraft:mineable/pickaxe'].Two direct cases in the existing loader suite cover a namespaced map, including an empty tag, and missing tags data. The per-version checks remain supplementary.
Validation:
npm test: 1,737 passing on the revised branch; 1,799 after a local integration with current master. This includes generation, lint and typing checks.Actual tags availability requires a release containing both this loader change and a data revision with tags. The data/schema work needs coordination with minecraft-data #1194 and #1308. This PR adds no datasets or tag-based mining behavior.