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

Improve ExprElement #5478

Merged
merged 19 commits into from
Jan 1, 2024
Merged

Improve ExprElement #5478

merged 19 commits into from
Jan 1, 2024

Conversation

UnderscoreTud
Copy link
Member

Description

This PR improves the ExprElement class and adds 2 new patterns to it:
The first pattern is [the] (first|:last) %number% elements [out] of %objects% which allows you to get the first or last x elements of a list.
Example:

set {_list::*} to "foo", "bar" and "foobar"
broadcast the first 2 elements of {_list::*} # "foo", "bar"

The second pattern is [the] elements (from|between) %number% (to|and) %number% [out] of %objects% which allows you to get elements between to different indices.
Example:

set {_list::*} to 10, 20, 50, 100
broadcast the elements from 2 to 4 of {_list::*} # 20, 50, 100

Target Minecraft Versions: any
Requirements: none
Related Issues: #5208

@UnderscoreTud UnderscoreTud added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Feb 28, 2023
@Fusezion
Copy link
Contributor

Fusezion commented Feb 28, 2023

Everything looks nice to me might be a good to add a few extra examples to it for the most part it's common but stuff like elements from 10 to 20 of {_list::*} aren't something some people will understand when they first interact with it

@Pikachu920
Copy link
Member

Could we please add ElementTypes specific to these new patterns instead of merging them together? When I added ElementType, the idea was that it would explain which element is being requested. For example, ElementType.MULTIPLE doesn't really explain what elements it corresponds to. It could be named ElementType.LAST_X_ELEMENTS or something, which is much more self explanatory.

@UnderscoreTud
Copy link
Member Author

I merged them because I feel like instead of having an enum for each mode + last variant, I just made it so the way we do "last" is with a boolean variable

It could be named ElementType.LAST_X_ELEMENTS or something

The ElementType.MULTIPLE could be from the first element or the last element depending on the last boolean.

Do you think I should just scrap the last boolean and instead add an enum for each type?

@Pikachu920
Copy link
Member

Do you think I should just scrap the last boolean and instead add an enum for each type?

That's what I would prefer personally!

# Conflicts:
#	src/main/java/ch/njol/skript/expressions/ExprElement.java
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Looks nice.

@TheLimeGlass TheLimeGlass added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Mar 15, 2023
@TheLimeGlass
Copy link
Collaborator

Adding bug tag as you currently CANNOT run debug mode with this class prior to this pull request.

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Jun 27, 2023

We need this pull request as it fixes this error. I keep coming back to this pull as it needs to be addressed, literally cannot run debug right now.

[02:43:38 ERROR]: #!#! Stack trace:
[02:43:38 ERROR]: #!#! java.lang.IllegalStateException
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.expressions.ExprElement.toString(ExprElement.java:172)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.conditions.CondCompare.toString(CondCompare.java:336)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.test.runner.EffAssert.toString(EffAssert.java:105)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.ScriptLoader.loadItems(ScriptLoader.java:939)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.lang.SkriptEvent.load(SkriptEvent.java:133)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.ScriptLoader.lambda$loadScripts$8(ScriptLoader.java:557)
[02:43:38 ERROR]: #!#!     at java.base/java.util.ArrayList.removeIf(ArrayList.java:1672)
[02:43:38 ERROR]: #!#!     at java.base/java.util.ArrayList.removeIf(ArrayList.java:1660)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.ScriptLoader.lambda$loadScripts$10(ScriptLoader.java:553)
[02:43:38 ERROR]: #!#!     at java.base/java.util.concurrent.CompletableFuture.uniApplyNow(CompletableFuture.java:684)
[02:43:38 ERROR]: #!#!     at java.base/java.util.concurrent.CompletableFuture.uniApplyStage(CompletableFuture.java:662)
[02:43:38 ERROR]: #!#!     at java.base/java.util.concurrent.CompletableFuture.thenApply(CompletableFuture.java:2168)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.ScriptLoader.loadScripts(ScriptLoader.java:513)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.ScriptLoader.loadScripts(ScriptLoader.java:459)
[02:43:38 ERROR]: #!#!     at Skript.jar//ch.njol.skript.Skript$1.lambda$run$3(Skript.java:666)
[02:43:38 ERROR]: #!#!     at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftTask.run(CraftTask.java:101)
[02:43:38 ERROR]: #!#!     at org.bukkit.craftbukkit.v1_19_R2.scheduler.CraftScheduler.mainThreadHeartbeat(CraftScheduler.java:483)
[02:43:38 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.b(MinecraftServer.java:1473)
[02:43:38 ERROR]: #!#!     at net.minecraft.server.dedicated.DedicatedServer.b(DedicatedServer.java:447)
[02:43:38 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.a(MinecraftServer.java:1397)
[02:43:38 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.w(MinecraftServer.java:1173)
[02:43:38 ERROR]: #!#!     at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:316)
[02:43:38 ERROR]: #!#!     at java.base/java.lang.Thread.run(Thread.java:833)

@UnderscoreTud UnderscoreTud changed the base branch from master to dev/feature September 17, 2023 09:54
- Change patterns to use %integer% rather than %number%
- Make use of generics to reduce the amount of castings needed
- Create a util method for getting a sub-array of an array
@sovdeeth sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Everything else seems great!

@UnderscoreTud UnderscoreTud merged commit 669ee87 into dev/feature Jan 1, 2024
5 checks passed
@UnderscoreTud UnderscoreTud deleted the enhancement/expr-element branch January 1, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants