For each index & value loop. #6562

Moderocky · 2024-04-13T11:06:35Z

Description

Having talked with Ayham about my concerns, I've opened this as an alternative proposal to Ayham's loop PR #6053.

This will be in draft until we reach a consensus about which approach to adopt.

Ayham's syntax was:

loop %objects% with index as %-~object%
loop %objects% [with index] as %-~object%( and|,) [with value] %-~object%
loop %objects% [[with value] as %-~object%]

I'm not very happy about changing or adding to the existing loop ... section syntax, because it's such an old and established thing, and I worried that the syntax was becoming a bit long and difficult to interpret.
I also thought this feature might be a good opportunity to mimic for-each loops from other languages like Java and Python.

My alternative for-each is specifically designed for dealing with key<->value maps,
by storing the index and value in reference variables.

It also extends the existing SecLoop so all loop behaviour is available as normal.

The syntax is (note ~object specifically accepts a variable):

for [each] [value] %~object% in %objects%
for [each] (key|index) %~object% in %objects%
for [each] [key|index] %~object% (=|and) [value] %~object% in %objects%

Note: while I wanted to do for A, B in ... (like Python) I was concerned that Skript might mistake this for for <list of A, B> in ... rather than for <A>, <B> in ....

set {_list::*} to 1, 5, and 10

# loop-by-value like a regular loop
for each {_value} in {_list::*}:
    broadcast {_value}

for value {_value} in {_list::*}:
    broadcast {_value}

# loop-by-index
for index {_index} in {_list::*}:
    broadcast {_index}

# loop by index = value
for index {_index} and value {_value} in {_list::*}:
    broadcast "%{_index}% = %{_value}%"

for {_index} and {_value} in {_list::*}:
    broadcast "%{_index}% = %{_value}%"

for {_index} = {_value} in {_list::*}:
    broadcast "%{_index}% = %{_value}%"

Note: this is registered as an experimental feature, toggled with using for each loops.

Target Minecraft Versions: any
Requirements: #6551, #6552
Related Issues: #6052, #6053

Pikachu920 · 2024-04-14T13:22:20Z

I'm still not sure about this feature. I think the existing system works well, and I think there's a benefit in just having a single simple way to loop things.

Moderocky · 2024-04-14T13:24:05Z

I'm still not sure about this feature. I think the existing system works well, and I think there's a benefit in just having a single simple way to loop things.

I'm happy with that too, but if people decide we need something I am in favour of this being it.

sovdeeth · 2024-04-14T13:27:36Z

Pretty much every loop I've written recently has looked like this, when more than one nested loop is involved.

loop 10 times:
    set {_counter} to loop-value
    loop {_something::*}:
        set {_value} to loop-value-2

It's just so much easier to keep track of what's what, helps avoid off-by-one errors in the loop-value-x, and being able to have descriptive names makes the code way more legible. That said, this code is perfectly fine to keep, but I feel like this addition would be valuable in that it would remove the dead weight of those set lines.

Moderocky · 2024-04-14T13:31:13Z

helps avoid off-by-one errors in the loop-value-x

I have a vague memory that there are some cases where you can't use the loop-<something>-x because it isn't available within some kinds of subsections (or something like that) and you have to copy it to a variable too.

sovdeeth · 2024-04-14T13:35:26Z

helps avoid off-by-one errors in the loop-value-x

I have a vague memory that there are some cases where you can't use the loop-<something>-x because it isn't available within some kinds of subsections (or something like that) and you have to copy it to a variable too.

Yes, any section that resets the events (EffSecSpawn, for example) means you have to put the loop-value into a var in order to access it in the section, since when it runs isn't guaranteed.

Moderocky · 2024-09-03T12:05:08Z

We voted for Sovde's suggestion to go with this version but allow loop as an alternative to for (each).

src/main/java/ch/njol/skript/sections/SecFor.java

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

src/main/java/ch/njol/skript/registrations/Feature.java

APickledWalrus · 2024-09-22T17:53:41Z

src/main/java/ch/njol/skript/sections/SecFor.java

+		Skript.registerSection(SecFor.class,
+			"(for [each]|loop) [value] %~object% in %objects%",
+			"(for [each]|loop) (key|index) %~object% in %objects%",
+			"(for [each]|loop) [key|index] %~object% (=|and) [value] %~object% in %objects%"


I'm not sure about this = syntax. Is there a reason you chose it?

I think it was either from one of the issues/discussions suggesting this or I was copying from some other language, It's been about six months and I can't really remember.

Oh I remember, I think there was a worry about {x} and {y} being confusingly-ambiguous (not in terms of parsing but for a user to read, maybe thinking it was looping two items at a time) between a regular list of expressions and this specific syntax where the and is literally part of it, and we wanted a less-ambiguous alternative that emphasised the keys were mapped to the values.
I'd rather it wasn't a symbol (=) although I don't mind it, but I couldn't think of a better single word for it.

I think we should avoid symbols if possible. I don't think for each {_a} = {_b} in {_c::*} is great.
In fact, we might want to require the usage of the keywords key|index and value for this option. for each {_a} and {_b} in {_c::*} makes it seem like c is holding some sort of tuple.

I'm fine with getting rid of =, how would you feel about , as an alternative? Python has e.g. for key, value in dict.items() and that's not terrible IMO.
I'm just worried that writing out the whole for key {blah} and value {blah} in ... is going to be more verbose than is necessary, when really we just need tome way to indicate one is the index representing the other.

That would certainly be a better shorthand than =. I am okay with it, though I dont think for {_key} and {_value} in ... is too bad

src/main/java/ch/njol/skript/sections/SecFor.java

Moderocky · 2024-11-03T11:21:32Z

src/main/java/ch/njol/skript/sections/SecFor.java

+		Skript.registerSection(SecFor.class,
+			"(for [each]|loop) [value] %~object% in %objects%",
+			"(for [each]|loop) (key|index) %~object% in %objects%",
+			"(for [each]|loop) [key|index] %~object% (=|and) [value] %~object% in %objects%"


I think it was either from one of the issues/discussions suggesting this or I was copying from some other language, It's been about six months and I can't really remember.

src/main/java/ch/njol/skript/registrations/Feature.java

Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>

APickledWalrus

I think the implementation is solid. My only concern is with the syntax.

APickledWalrus · 2024-11-20T00:37:32Z

src/main/java/ch/njol/skript/sections/SecFor.java

+		Skript.registerSection(SecFor.class,
+			"(for [each]|loop) [value] %~object% in %objects%",
+			"(for [each]|loop) (key|index) %~object% in %objects%",
+			"(for [each]|loop) [key|index] %~object% (=|and) [value] %~object% in %objects%"


I think we should avoid symbols if possible. I don't think for each {_a} = {_b} in {_c::*} is great.
In fact, we might want to require the usage of the keywords key|index and value for this option. for each {_a} and {_b} in {_c::*} makes it seem like c is holding some sort of tuple.

APickledWalrus

The implementation looks fine. My only concern is the syntax (see my existing comments)

src/main/java/ch/njol/skript/sections/SecFor.java

APickledWalrus

Good work

APickledWalrus · 2024-12-16T16:54:41Z

src/main/java/ch/njol/skript/sections/SecLoop.java

 			return walk(event, true);
 		}
 	}

+	protected void store(Event event, Object next) {
+		this.current.put(event, next);
+		this.currentLoopCounter.put(event, (currentLoopCounter.getOrDefault(event, 0L)) + 1);


Suggested change

this.currentLoopCounter.put(event, (currentLoopCounter.getOrDefault(event, 0L)) + 1);

this.currentLoopCounter.put(event, currentLoopCounter.getOrDefault(event, 0L) + 1);

The parentheses use seems a bit odd here

APickledWalrus · 2024-12-16T16:56:40Z

src/main/java/ch/njol/skript/sections/SecFor.java

+			Skript.error("The 'key' input for a for-loop must be a variable to store the value.");
+			return false;
+		}
+		if (!(valueStore instanceof Variable || valueStore == null)) {


Suggested change

if (!(valueStore instanceof Variable || valueStore == null)) {

if (valueStore != null && !(valueStore instanceof Variable)) {

Just to bring it in line with the keyStore check (and I think this is clearer)

src/main/java/ch/njol/skript/sections/SecFor.java

Efnilite · 2024-12-16T17:17:18Z

src/main/java/ch/njol/skript/sections/SecFor.java

+		}
+		if (Container.class.isAssignableFrom(expression.getReturnType())) {


Suggested change

}

if (Container.class.isAssignableFrom(expression.getReturnType())) {

}

if (Container.class.isAssignableFrom(expression.getReturnType())) {

Efnilite · 2024-12-16T17:17:24Z

src/main/java/ch/njol/skript/sections/SecFor.java

+		}
+		if (!LiteralUtils.canInitSafely(expression)) {


Suggested change

}

if (!LiteralUtils.canInitSafely(expression)) {

}

if (!LiteralUtils.canInitSafely(expression)) {

Efnilite · 2024-12-16T17:17:39Z

src/main/java/ch/njol/skript/sections/SecFor.java

+		}
+		if (!(valueStore instanceof Variable || valueStore == null)) {


Suggested change

}

if (!(valueStore instanceof Variable || valueStore == null)) {

}

if (!(valueStore instanceof Variable || valueStore == null)) {

Efnilite · 2024-12-16T17:17:55Z

src/main/java/ch/njol/skript/sections/SecFor.java

+			return false;
+		//<editor-fold desc="Set the key/value expressions based on the pattern" defaultstate="collapsed">


Suggested change

return false;

//<editor-fold desc="Set the key/value expressions based on the pattern" defaultstate="collapsed">

return false;

//<editor-fold desc="Set the key/value expressions based on the pattern" defaultstate="collapsed">

Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>

Moderocky changed the title ~~For each index & value loop~~ For each index & value loop. Apr 13, 2024

AyhamAl-Ali added the feature Pull request adding a new feature. label Apr 13, 2024

Moderocky added 3 commits September 3, 2024 12:54

Pick first.

3cd421e

Export behaviour from SecLoop instead.

ba3be6f

Fix changes from upstream.

36c1e09

Moderocky force-pushed the for-loop branch from 34f7505 to 36c1e09 Compare September 3, 2024 12:03

Moderocky added the 2.10 Targeting a 2.10.X version release label Sep 3, 2024

Moderocky requested review from sovdeeth, APickledWalrus, UnderscoreTud and AyhamAl-Ali September 3, 2024 12:04

Moderocky marked this pull request as ready for review September 3, 2024 12:04

sovdeeth approved these changes Sep 4, 2024

View reviewed changes

src/main/java/ch/njol/skript/sections/SecFor.java Outdated Show resolved Hide resolved

src/main/java/ch/njol/skript/sections/SecFor.java Show resolved Hide resolved

Update src/main/java/ch/njol/skript/sections/SecFor.java

8c61221

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

Moderocky mentioned this pull request Sep 13, 2024

Keyed Set Mode #7072

Merged

sovdeeth mentioned this pull request Sep 15, 2024

🚀 Add loop variable reference (loop X as INDEX and VALUE) #6053

Closed

APickledWalrus requested changes Sep 22, 2024

View reviewed changes

Moderocky commented Nov 3, 2024

View reviewed changes

Apply suggestions from code review

1000200

Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>

Moderocky requested a review from APickledWalrus November 3, 2024 11:25

APickledWalrus requested changes Nov 20, 2024

View reviewed changes

Moderocky requested a review from APickledWalrus November 29, 2024 10:07

Merge branch 'dev/feature' into for-loop

9ca6aa5

APickledWalrus requested changes Dec 16, 2024

View reviewed changes

src/main/java/ch/njol/skript/sections/SecFor.java Show resolved Hide resolved

Moderocky added 2 commits December 16, 2024 07:23

Merge branch 'dev/feature' into for-loop

edf1230

Fix problems from merge.

ebb6365

Moderocky requested a review from APickledWalrus December 16, 2024 06:28

APickledWalrus approved these changes Dec 16, 2024

View reviewed changes

Efnilite approved these changes Dec 16, 2024

View reviewed changes

Update src/main/java/ch/njol/skript/sections/SecFor.java

7f8d74a

Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>

Moderocky added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Dec 16, 2024

Merge branch 'dev/feature' into for-loop

0eb2802

Moderocky merged commit 3bcee98 into SkriptLang:dev/feature Dec 18, 2024
5 checks passed

Moderocky deleted the for-loop branch December 18, 2024 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For each index & value loop. #6562

For each index & value loop. #6562

Moderocky commented Apr 13, 2024

Pikachu920 commented Apr 14, 2024

Moderocky commented Apr 14, 2024

sovdeeth commented Apr 14, 2024

Moderocky commented Apr 14, 2024

sovdeeth commented Apr 14, 2024

Moderocky commented Sep 3, 2024

APickledWalrus Sep 22, 2024

Moderocky Nov 3, 2024

Moderocky Nov 3, 2024

APickledWalrus Nov 20, 2024

Moderocky Nov 20, 2024

APickledWalrus Dec 3, 2024 •

edited

Loading

Moderocky Nov 3, 2024

APickledWalrus left a comment

APickledWalrus Nov 20, 2024

APickledWalrus left a comment

APickledWalrus left a comment

APickledWalrus Dec 16, 2024

APickledWalrus Dec 16, 2024

Efnilite Dec 16, 2024

Efnilite Dec 16, 2024

Efnilite Dec 16, 2024

Efnilite Dec 16, 2024

	this.currentLoopCounter.put(event, (currentLoopCounter.getOrDefault(event, 0L)) + 1);
	this.currentLoopCounter.put(event, currentLoopCounter.getOrDefault(event, 0L) + 1);

	if (!(valueStore instanceof Variable \|\| valueStore == null)) {
	if (valueStore != null && !(valueStore instanceof Variable)) {

		}
		if (Container.class.isAssignableFrom(expression.getReturnType())) {

		return false;
		//<editor-fold desc="Set the key/value expressions based on the pattern" defaultstate="collapsed">

For each index & value loop. #6562

For each index & value loop. #6562

Conversation

Moderocky commented Apr 13, 2024

Description

Pikachu920 commented Apr 14, 2024

Moderocky commented Apr 14, 2024

sovdeeth commented Apr 14, 2024

Moderocky commented Apr 14, 2024

sovdeeth commented Apr 14, 2024

Moderocky commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus Dec 3, 2024 •

edited

Loading