✨ Add support for item components #1131
Conversation
these could be spun into separate tickets maybe? or at least once this PR is merged we should update the original ticket #1118 with the progress of item components and what needs to still be done, etc. |
MulverineX
changed the title
There was a problem hiding this comment.
Can you merge the main branch in and add some tests in packages/java-edition/test/mcfunction/parser/argument.spec.ts to document the parse tree? You might need to add some logic in the test file so that both the old item parser and the new item parser can be tested.
| if (components.length > 1) { | ||
| components.forEach(component => { | ||
| ctx.err.report( | ||
| localize('duplicate-components', componentName), | ||
| component.key!.range, | ||
| core.ErrorSeverity.Error, | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
You can use core.LanguageErrorInfo.related to report only one error for this with the duplicated keys' locations added in the info.related field (core.Location.create() would be helpful to create the entries for the field)
There was a problem hiding this comment.
Vscode also creates an error for each duplicate key in a json file for example, imo this is the most clean
| sep: [',', '|'], | ||
| end: ']', | ||
| trailingSep: true, |
There was a problem hiding this comment.
This would allow trailing |'s, although upon my testing trailing commas aren't allowed by the game in item predicates (but they are allowed in non predicate), so just removing trailingSep: true probably works (would be nice if a comment could be added there to explain this). The parse tree also won't reflect the real hierarchy of the tests but I guess that doesn't matter much for us.
There was a problem hiding this comment.
The parse tree you see barely functions as is, unfortunately. There's (supposed to) be a push to fix it when I have the time
Sure, once the parser is fixed and the PR is ready to incorporate all the reviews |
|
I did some refactoring. It is mostly done now apart from support for |
|
The PR should be ready for final reviews now |
| range: core.Range.get(range), | ||
| children: [id], | ||
| id, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export interface ComponentTestsNode extends core.AstNode { | ||
| type: 'mcfunction:component_tests' | ||
| children: ComponentTestsAnyOfNode[] |
There was a problem hiding this comment.
I think there is a layer of children too much here. This children field will always have exactly zero or one child. Maybe the best way to solve this is by getting rid of this node, and using ComponentTestsAnyOfNode in ItemPredicateNode
There was a problem hiding this comment.
Originally ComponentTestsAnyOfNode was being used directly, but IMO encapsulating it in a ComponentTestsNode is more readable, and potentially less prone to breakage if the representation for the AST changes
There was a problem hiding this comment.
I disagree with that. We don't need unnecessary wrapper nodes. It also makes things like checkers and completers more complex for no reason.
There was a problem hiding this comment.
Hmm I'm not so sure. My main concern is future-proofing, since having things refer to ComponentTestsNode seems better (and more understandable at a glance to new devs) than having to change all references from ComponentTestsAnyOfNode to MaybeSomethingMoreComplicatedNode[] if that ever happens. I doubt it's a big deal in either case so I'm OK with the change.
This PR lays the foundation for supporting the new Item Component format Mojang introduced in
1.20.5.There is still work to be done - for now, this PR is a draft but almost ready for merging.
Done
give @s cooked_beef[food={nutrition:5,saturation:2}]give @s stick[food=...,food=...]execute if|unlessandclearWhat's left (outside this PR's scope)