Refactor ExprBeaconValues into multiple expressions, fix bugs. #7729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sovdeeth
added
bug
Efnilite
approved these changes
Mar 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
ExprBeaconValues is an expression that tries to do too many things at once. Since it's trying to handle potion effects, range, and tier all at once, it is very large and overly complex for what it needs to be. This complexity led to multiple bugs not being caught in testing or review, like failing to check if the block state is a beacon before casting it and returning Object.class in acceptChange, which led to lines like
set primary effect of {block} to "hello world"being parsed with no errors.This PR splits the expression into its component parts (ExprBeaconEffects, ExprBeaconTier, ExprBeaconRange) which allows the code to be much simpler in each class. The bugs mentioned above are fixed.
There is a breaking change in the syntax,
[beacon] range/tier->beacon range/tieras I thought that the expressions were too generic without thebeaconprefix. Let me know if you disagree or think it not worth changing. The DELETE mode was also removed from the range expression, as deleting a range doesn't make much sense to me.Target Minecraft Versions: any
Requirements: none
Related Issues: none