-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
Fix the legendary compare issue with two same literals #5503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice, are you sure it won't break anything else?
Yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any problems with this.
Co-authored-by: Kenzie <admin@moderocky.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd revert the other stuff as it's not in the scope of this pr or related to it in anyway
Also, tests are failing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done ⚡
Code wise looks good, I haven't made enough tests though so I prefer this gets more testing by the community.
src/test/java/org/skriptlang/skript/test/tests/regression/HangingEntity_4871.java
Outdated
Show resolved
Hide resolved
src/test/java/org/skriptlang/skript/test/tests/regression/HangingEntity_4871.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: _tud <mmbakkar06@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Would be good to have for testing in beta3. Nice work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good ⚡
Description
Fixes the legendary compare issue with two same literals. Example: Skript would set
fire
as visual effect, and then if you went to compare it against a block it would error. Skript should be assumingfire
is an itemtype and compare against it. More details at #5497I also added file context to the EffAssert because I had many tests fail during testing and needed to know which files they belong to.
I also added event-block and event-location to the test event so that it can match the JUnit values.
Target Minecraft Versions: any
Requirements: none
Related Issues: #2711, #4773, #5497, #5557 and #5675 there are probably more I couldn't find.
duplicates: #4769, #4774, #4779, #4809