-
Notifications
You must be signed in to change notification settings - Fork 5
feat: support variable parameter counts with type validation in molang queries #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@Xterionix This looks okay, what do you think? |
|
Pretty close to what I wanted. Issue with just accounting for minParameter is that I was hoping to validate the value used. Invalid Valid |
@copilot can you come up with a solutions for this? that we still want to be able to support an N amount of parameters that either have to be more molang, or strings/int/bools |
Added type validation for variable parameters in commit b8b406b. The solution introduces a {
id: 'block_has_all_tags',
parameters: [
{ id: 'x', type: 'float' },
{ id: 'y', type: 'float' },
{ id: 'z', type: 'float' },
{ id: 'tag', type: 'string' },
],
minParams: 4,
repeatableParam: { id: 'tag', type: 'string' }
}Now |
|
I do think having to specify the type again is redundant. But if it works, it works I suppose. Would be better imo if you just could directly do something like parameters: [
...
{ id: 'tag', type: 'string', repeatable: true },
]or parameters: [
...
{ id: 'tag', type: 'string' }
]
repeatable: 'tag' // Id of param that is being repeated |
|
@copilot I think we need to refactor, whereby we now do things differently for the field parameters: Implement the following new structure: {
"parameters": [
{ "id": "tag 1", "type": "string" },
{ "id": "tag 2", "type": "string" },
{ "id": "tag 3", "type": "string", "repeatable": true }
]
} |
Refactored to use the new structure in commit 12c4dc5. The implementation now uses {
parameters: [
{ id: "slot_name", type: "string" },
{ id: "tag", type: "string", repeatable: true }
]
}The diagnoser automatically detects if the last parameter is repeatable and validates accordingly. All 530 tests pass. |
|
@Xterionix I had copilot combine your thoughts and thinking it might have been more suitable as a field on the last parameter. Since its only the last parameter that is allowed to be repeated 🤔 Im happy enough to approve this and merge it if you are |
Co-authored-by: DaanV2 <[email protected]>
Co-authored-by: DaanV2 <[email protected]>
Co-authored-by: DaanV2 <[email protected]>
…repeatableParam fields Co-authored-by: DaanV2 <[email protected]>
12c4dc5 to
06dbb48
Compare
|
Looks good to me. And yeah that makes sense: last field is the only one that'll ever be repeatable |
Issue/Feature Request/Bug report
Changes
Molang queries like
query.is_owner_identifier_anyandquery.equipped_item_all_tagsaccept variable numbers of arguments (n ≥ minimum), but the diagnoser was enforcing exact parameter counts and not validating parameter types.Core data model (
packages/molang/src/data/molang-function.ts):repeatable?: booleanfield toMolangParameterinterface to mark parameters that can be repeated indefinitelyminParamsandrepeatableParamfields fromMolangFunctioninterface in favor of simpler per-parameter approachQuery definitions (
packages/molang/src/data/general.ts):repeatable: trueon the last parameter:is_owner_identifier_any,is_name_any(type: string, repeatable)equipped_item_all_tags,equipped_item_any_tag(slot: string, tag: string repeatable)block_has_all_tags,block_neighbor_has_all_tags, etc (xyz: float, tag: string repeatable)graphics_mode_is_any,last_input_mode_is_any(mode: string repeatable)is_item_name_any(slot: string, index: float, item: string repeatable)entity_biome_has_all_tags,entity_biome_has_any_tags, etc (tag/identifier: string repeatable)Diagnostics (
packages/bedrock-diagnoser/src/diagnostics/molang/expressions.ts):repeatable: trueparameters.lengthExamples
Before:
After:
New structure:
Checklist
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.