Skip to content
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

feat: add item predicate syntax and inline loot arguments #297

Closed
wants to merge 6 commits into from

Conversation

misode
Copy link
Contributor

@misode misode commented Apr 3, 2024

This PR adds support for the new item predicate syntax.

*[custom_data~{gm4_metallurgy:{stored_shamir:"lumos"}},damage|!max_item_stack=1]

I would like some feedback, mostly on the AST structure and maybe on the parser (could it be split up, does it need anything special to support multiline?)

Also the tests are failing, probably because I changed the AST result of the item_predicate format, it's no longer an AstItem, but now an AstItemPredicate. Would it be better to have a single AST that supports both?

@misode misode changed the title feat: add item predicate syntax feat: add item predicate syntax and inline loot arguments Apr 3, 2024
@RitikShah
Copy link
Member

Did you make AstItemPredicate represent both what AstItem and the new inline predicates do now? I'm a bit confused, isn't inline predicates an alternative to the previous way, so shouldn't it be a union of both things, or is this what you are asking about.

@misode
Copy link
Contributor Author

misode commented Apr 4, 2024

Yeah that's the bulk of the problem. The issue is that item predicates allow a complex syntax including |, ~, etc. Item predicates need to be able to represent this complex structure. Maybe components in AstItem could be a Union[AstItemComponents, AstItemComponentsGroup]

@misode misode force-pushed the 1.20.5-item-predicate branch 3 times, most recently from fa0b4e6 to 7395ba4 Compare April 4, 2024 22:13
@misode
Copy link
Contributor Author

misode commented Apr 4, 2024

I'm happy with the PR now. Feel free to review @vberlier @RitikShah

Copy link
Member

@RitikShah RitikShah left a comment

Choose a reason for hiding this comment

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

I'm not as familiar with the Mecha codebase but this implementation from the last commit seems cleaner.

yield node.value

@rule(AstItemTestGroup)
def item_test_group(self, node: AstItemTestGroup, result: List[str]):
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this implementation. what's with sep and comma here?

Copy link
Member

Choose a reason for hiding this comment

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

ohh, it's just adjusting what's outputted based on how many items need to be outputted..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly took this from self.collection, but that always requires some kind of surrounding brackets and uses commas as separators, which was too limiting

vberlier added a commit that referenced this pull request Apr 10, 2024
@vberlier
Copy link
Member

vberlier commented Apr 11, 2024

After looking at the PR and investigating the official changelog I thought it would probably be more useful down the line if we split AstItem into separate node types AstItemStack and AstItemPredicate, so that's what I did and released in v0.93.0. The final design ended up quite similar to your first commit actually. I couldn't merge as-is but your changes served as a great starting point, thanks again for the help!

@vberlier vberlier closed this Apr 11, 2024
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.

3 participants