Skip to content

Conversation

@BrandonChanSkybox
Copy link

Adds Metadata support for Property and Argument Minimum and Maximum bounds info

Copy link
Contributor

@SBLMikeDemone SBLMikeDemone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few unnecessary changes and some mild confusion around the new argument comments

}
}
},{
"has_max": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we drop the has_min/has_max flags and just rely on the min/max being present?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. By default, our bounds are set to a Type's numeric limits, (the absolute minimum and maximum values). Because of that, min/max will almost always be present. The has_min and has_max flag distinguish whether the min_value and max_value provided are different from the numeric limits of the Type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

                {
                    ...
                    "has_max": true,
                    "has_min": false,
                    "max_value": 255,
                    "min_value": -2147483648, // Can we remove automatically added limits like this?
                    "type": {
                        ...
                        "valid_range": {
                            "max": 2147483647,
                            "min": -2147483648
                        }
                    }
                }

I wonder if we should change that. Types already have a "valid range" right? Is there a reason that we always stamp down "min_value" and "max_value" even if a binding doesn't specifically add limits?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we chatted about this, the valid range is wrong, it's hardcoded to int min/max. We were gonna do a follow up that fixes that and then gets rid of has_max/has_min.

Alternatively, I find it weird that "type" even has a "valid_range". The generator could just have a map of type -> valid range and that'd get rid of quite a lot of metadata.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made follow up here: Task 1527387: [Infra] [Scripting] Calculate has_min and has_max value in metadata

@BrandonChanSkybox We'll need to validate this change works in MC and then merge it into MC, and it may make more sense to just do all this in 1 PR after you're back in vacation. It'll result in less metadata changes over time and less updates to the generator. What do you think?

Copy link
Author

@BrandonChanSkybox BrandonChanSkybox Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I removed the usage of has_min and has_max.
Now we just check if the min_value and max_value has been populated instead. A matching change was made in the other repo's PR to make sure we aren't creating min_value or max_value parameters when they aren't necessary, like the automatically added limits Jake mentioned

Copy link
Contributor

@SBLMikeDemone SBLMikeDemone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, just 2 small nits.

Copy link
Contributor

@zachcampbellskyboxlabs zachcampbellskyboxlabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Make sure to update the snapshots with the Bounds: [x, y] change!

Copy link
Contributor

@SBLMikeDemone SBLMikeDemone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Nothing blocking, thank you for the tests!

for (const scriptModule of release.script_modules) {
const classJson = scriptModule.classes ?? [];
const interfaceJson = scriptModule.interfaces ?? [];
const concatJsonArray: (MinecraftInterface | MinecraftClass)[] = classJson.concat(interfaceJson);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you might be able to just do something like:

const concatJsonArray: { properties: MinecraftProperty[], functions: MinecraftFunction[]}[] = classJson.concat(interfaceJson);

Then get rid of the _getFunctionsForInterfaceOrClass and _getPropertiesForInterfaceOrClass

{{/details.default_value}}
{{/has_defaults}}
{{#has_bounds}}
* Bounds: [{{details.min_value}} , {{details.max_value}}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: Is the min_value and max_value inclusive or exclusive? Square brackets normally means inclusive, i.e. [0, 2] would be 0, 1, 2, and (0, 2) would just be 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants