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.

Sign up for GitHub

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

feature: ExecExprListOperators #81

Merged
merged 6 commits into from
Nov 3, 2020
Merged

feature: ExecExprListOperators #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b76c25
fix: ClassCastException in ExecutableExpression
Oct 16, 2020
7100fd6
feature: ExecExprListOperators
Oct 16, 2020
65eebef
improve: small code improvements
Oct 24, 2020
6f6449d
improve: more options in ExecExprListOperators
Nov 1, 2020
418f142
improve: allow negative step and fix other issues
Nov 2, 2020
373139f
fix: leftover debug message
Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 211 additions & 0 deletions src/main/java/io/github/syst3ms/skriptparser/expressions/ExecExprListOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
package io.github.syst3ms.skriptparser.expressions;

import io.github.syst3ms.skriptparser.Parser;
import io.github.syst3ms.skriptparser.lang.Expression;
import io.github.syst3ms.skriptparser.lang.TriggerContext;
import io.github.syst3ms.skriptparser.lang.base.ExecutableExpression;
import io.github.syst3ms.skriptparser.log.ErrorType;
import io.github.syst3ms.skriptparser.parsing.ParseContext;
import io.github.syst3ms.skriptparser.types.changers.ChangeMode;
import io.github.syst3ms.skriptparser.util.CollectionUtils;
import org.jetbrains.annotations.Nullable;

import java.math.BigInteger;
import java.util.ArrayList;
import java.util.Arrays;

/**
* Basic list operators with the following behavior:
* <ul>
* <li>{@code pop} removes the last element and returns that removed element.</li>
* <li>{@code shift/poll} removes the first element and returns that removed element.</li>
* <li>{@code extract} removes a specific (or just the first/last element) and returns that removed element</li>
* <li>{@code splice} removes elements in a certain bound and returns those removed elements.</li>
* </ul>
Mwexim marked this conversation as resolved.
Show resolved Hide resolved
* These are all changing operators, which mean they apply the operator on the source list
* and return other elements accordingly to the operator type. If the list is not changeable,
* then these operators will not work.
* Note that indices in Skript start at one and that both the lower and upper bounds are inclusive.
* The step function can be used in the {@code splice} pattern to skip over certain values. Note that
* when a negative step function is used, the list is reversed as well as the lower and upper bounds,
* which means the lower bound must be higher than the upper bound.
*
* @name List Operators
* @type EXPRESSION
* @pattern pop %objects%
* @pattern (shift|poll) %objects%
* @pattern splice %objects% (from %integer% to %integer%|starting (at|from) %integer%|up to %integer%) [[with] step %integer%]
* @since ALPHA
* @author Mwexim
* @see ExprElement
*/
public class ExecExprListOperators extends ExecutableExpression<Object> {
static {
Parser.getMainRegistration().addSelfRegisteringElement(
ExecExprListOperators.class,
Object.class,
false,
"extract [the] (0:last|1:first|2:%integer%(st|nd|rd|th)) element out [of] %objects%",
"pop %objects%",
"(shift|poll) %objects%",
"splice %objects% (0:from %integer% to %integer%|1:starting (at|from) %integer%|2:up to %integer%) [[with] step %integer%]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the splice keyword be mandatory? Sure, for an effect it wouldn't make sense, but for an expression I wonder whether splicing is common enough for it to be required. I'm not sure.

Having "until" as an alternative to "up to" would also be desirable, because (1) it's a common English construction, and (2) it's less unwieldy when using negative steps.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think removing the splice keyword might prove to be more unwieldy than before. Without the keyword, no one really knows what this method does. People may think that it just returns the values in that range, but they also get removed from the list, which may not be obvious at first.

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a way to splice a list without actually changing it. Perhaps with two separate classes? I don't really like non-obvious side-effects in expressions.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It'd be non-obvious if we'd remove the splice keyword. Now people know that this happens. We can add these expressions later in a separate class or add them to ExprMutableList for example, but that's currently something that I will not add in this particular PR.

);
}

private int type;
private Expression<Object> list;
private Expression<BigInteger> index;
private Expression<BigInteger> lower;
private Expression<BigInteger> upper;
private Expression<BigInteger> step;

@SuppressWarnings("unchecked")
@Override
public boolean init(Expression<?>[] expressions, int matchedPattern, ParseContext parseContext) {
switch (matchedPattern) {
case 0:
type = parseContext.getParseMark();
list = (Expression<Object>) (type == 2 ? expressions[1] : expressions[0]);
if (type == 2)
index = (Expression<BigInteger>) expressions[0];
break;
case 1:
case 2:
type = matchedPattern - 1;
list = (Expression<Object>) expressions[0];
break;
case 3:
type = 3;
list = (Expression<Object>) expressions[0];
switch (parseContext.getParseMark()) {
case 0:
lower = (Expression<BigInteger>) expressions[1];
upper = (Expression<BigInteger>) expressions[2];
if (expressions.length == 4)
step = (Expression<BigInteger>) expressions[3];
break;
case 1:
lower = (Expression<BigInteger>) expressions[1];
if (expressions.length == 3)
step = (Expression<BigInteger>) expressions[2];
break;
case 2:
upper = (Expression<BigInteger>) expressions[1];
if (expressions.length == 3)
step = (Expression<BigInteger>) expressions[2];
break;
default:
throw new IllegalStateException();
}
}
var logger = parseContext.getLogger();
if (list.acceptsChange(ChangeMode.SET).isEmpty()) {
logger.error(
list.toString(null, logger.isDebug()) + " is static and cannot be changed",
ErrorType.SEMANTIC_ERROR
);
return false;
} else if (list.isSingle()) {
logger.error(
list.toString(null, logger.isDebug()) + " represents a single value",
ErrorType.SEMANTIC_ERROR,
"List operators only work on multiple objects at the same time"
);
return false;
}
return true;
}

@Override
public Object[] getValues(TriggerContext ctx) {
var values = list.getValues(ctx);
if (values.length == 0)
return new Object[0];

switch (type) {
case 0:
list.change(ctx, Arrays.copyOfRange(values, 0, values.length - 1), ChangeMode.SET);
return new Object[] {values[values.length - 1]};
case 1:
list.change(ctx, Arrays.copyOfRange(values, 1, values.length), ChangeMode.SET);
return new Object[] {values[0]};
case 2:
int ind = index.getSingle(ctx)
.filter(n -> n.signum() > 0 && n.compareTo(BigInteger.valueOf(values.length)) <= 0)
.map(n -> n.intValue() - 1)
.orElse(-1);
if (ind == -1) {
return new Object[0];
}
var skipped = new Object[values.length - 1];
for (int i = 0, k = 0; i < values.length; i++) {
if (i == ind) {
continue;
}
skipped[k++] = values[i];
}
list.change(ctx, skipped, ChangeMode.SET);
return new Object[] {values[ind]};
case 3:
var low = lower != null
? lower.getSingle(ctx).filter(n -> n.signum() > 0).map(n -> n.intValue() - 1).orElse(-1)
: 0;
var up = upper != null
? upper.getSingle(ctx).filter(n -> n.compareTo(BigInteger.valueOf(values.length)) <= 0).map(BigInteger::intValue).orElse(values.length)
: values.length;
var st = step != null
? step.getSingle(ctx).filter(n -> n.signum() != 0 && n.compareTo(BigInteger.valueOf(-values.length)) >= 0 && n.compareTo(BigInteger.valueOf(values.length)) <= 0).map(BigInteger::intValue).orElse(0)
: 1;

if (st < 0) {
CollectionUtils.reverseArray(values);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, unfortunately that doesn't work. Take the list a = [0,1,2,3,4,5].
Using python syntax for splicing, a[4:1:-2] = [4,2]. However, reversing the list gives b = [5,4,3,2,1,0], and b[0:5:2] = [5,3,1]. I may be wrong about this due to indexing shenanigans, but please test it and make sure it works for larger samples either way.

This can probably be solved using some dirty modulo stuff, but honestly you'd be better off just trying to start at the given index, move by st every time, and check if you're still in the bounds at each step.

Copy link
Owner Author

Choose a reason for hiding this comment

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

In the lines under that statement I normally switched the lower and upper bound to provide the behavior you just specified.
I'll test this again tomorrow but I'm fairly sure I did it right, because I tested this myself before pushing. But we'll see.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Tested it, it works as intended. The only difference is that you'll need to use splice {list::*} from 5 to 2 with step -2 to have the result in your example, because:

  1. Indices in Skript start at 1.
  2. The upper bound is inclusive.

st = -st;
int temp = low;
low = up - 1;
up = temp + 1;
}
if (low == -1 || up == -1 || up == 0 || st == 0 || low > up) {
list.change(ctx, new Object[0], ChangeMode.SET);
return new Object[0];
} else if (low == up) {
// Nothing to change
return new Object[0];
}

var spliced = new ArrayList<>();
var changed = new ArrayList<>();
for (int i = 0; i < values.length; i++) {
if (i >= low && i < up && (i - low) % st == 0) {
spliced.add(values[i]);
} else {
changed.add(values[i]);
}
}

list.change(ctx, changed.toArray(), ChangeMode.SET);
return spliced.toArray(new Object[0]);
default:
throw new IllegalStateException();
}
}

@Override
public boolean isSingle() {
return type == 0 || type == 1;
}

@Override
public String toString(@Nullable TriggerContext ctx, boolean debug) {
switch (type) {
case 0:
return "pop " + list.toString(ctx, debug);
case 1:
return "poll " + list.toString(ctx, debug);
case 2:
return "splice " + list.toString(ctx, debug) + " from " + lower.toString(ctx, debug)
+ (upper != null ? upper.toString(ctx, debug) : "");
default:
throw new IllegalStateException();
}
}
}
2 changes: 1 addition & 1 deletion src/main/java/io/github/syst3ms/skriptparser/lang/base/ExecutableExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ protected void execute(TriggerContext ctx) {
public void register(SkriptRegistration reg, Object... args) {
Class<T> type = (Class<T>) args[0];
boolean isSingle = (boolean) args[1];
String[] patterns = (String[]) Arrays.copyOfRange(args, 2, args.length);
String[] patterns = Arrays.copyOfRange(args, 2, args.length, String[].class);

// The actual registration
reg.addExpression(getClass(), type, isSingle, patterns);
Expand Down