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

feature: ExecExprListOperators #81

Merged
merged 6 commits into from
Nov 3, 2020
Merged

feature: ExecExprListOperators #81

merged 6 commits into from
Nov 3, 2020

Conversation

Mwexim
Copy link
Owner

@Mwexim Mwexim commented Oct 16, 2020

This pull request adds two more syntaxes from issue #40 . The syntax is based around the poll() method in the Queue class, and together with the pop() and splice() methods in Javascript, this creates a nice little implementation of the ExecutableExpression utility class.

Furthermore, I fixed one critical oversight in the ExecutableExpression class I totally overlooked at first (and I just realised it when creating this implementation).

@Olyno
Copy link
Contributor

Olyno commented Oct 17, 2020

Nice job! I think the splice pattern should be included in the #62 because it takes an array to returns another (like others patterns in this pull request). We should make here an expression that takes an array and returns only 1 element (like pop pattern)

@Mwexim
Copy link
Owner Author

Mwexim commented Oct 17, 2020

Nice job! I think the splice pattern should be included in the #62 because it takes an array to returns another (like others patterns in this pull request). We should make here an expression that takes an array and returns only 1 element (like pop pattern)

The splice pattern takes in a range and cuts that range in the array. The cut part is returned, but the array still gets changed. Your pull request focusses more on sorting the array, and returning the sorted array. That’s not what these patterns do. And even then, the purpose of these patterns do not fit in your pull request in my opinion.
Thanks for the feedback!

@Mwexim Mwexim requested a review from Olyno October 26, 2020 18:22
"pop %objects%",
"(shift|poll) %objects%",
"splice %objects% from %integer% [1:to %integer%]"
"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.

: 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.

@Mwexim Mwexim merged commit 5c71347 into master Nov 3, 2020
@Mwexim Mwexim deleted the feature/queue-syntaxes branch November 3, 2020 22:44
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