Skip to content

While All/Any #7282

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

Closed
Closed

Conversation

Absolutionism
Copy link
Contributor

@Absolutionism Absolutionism commented Dec 19, 2024

Description

This PR aims to add AND conditions + OR conditions to SecWhile. Allowing users to define multiple conditions to check if the while loop should continue.


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

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Some early thoughts. I like the idea 🙂

@Moderocky Moderocky marked this pull request as draft December 19, 2024 17:39
@Moderocky
Copy link
Member

While this looks like a good start, I'm going to mark this as draft currently because there are some methodological things to think about (e.g. we have if any/all and while any/all, are there ways we can re-use parts of the multiline condition code? Are there other sections/conditions that might benefit from having something like an any/all? etc.) and it might be worth exploring this a bit more to see if there's anything fancy we can do :)

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall.

One (cursed) change we might want to make is with do while.
Rather than:

do while:
  a
  b
do:
  # code

We might have...

do:
  # code
while:
  a
  b

I'd like to hear what others think.

@Absolutionism
Copy link
Contributor Author

@APickledWalrus Just wanted to touch base a little.
For your suggestion, personally I'm not a fan of having the conditions being defined after the section to execute. Mainly because it doesn't fit. Implementing it wouldn't be hard. But say I have like 20 lines within the code to run, then the conditions afterward makes it seem awkward.
However, I do understand the ambiguity of having the [:do] and the actual do section. Maybe we can just come up with an alternative syntax, preferably for the newly added do section to avoid breaking changes, or if you think having this minor breaking change is "ok" then that works too. Some alternatives I was thinking of are: run, execute, trigger. But obviously these are already used elsewhere, leaving words like: perform, process, start, operate, etc. Which some may be fine, others not so much.

Also, regarding @Moderocky 's comment. I would like to say I've fully met the conditions. As only some parts are reused between both SecWhile and SecConditional, and the rest are unique to their behavior. Whether or not the methods I added are good or not, well is up to the teams discretion. The methods I added should cover a general basis if any elements were capable of implementing an any/all, presuming it doesn't fall out of scope of these methods, which then would be unique to an element and/or if shared, then a new method could be added.

Anyways, that about sums it up. Let me know your thoughts.

@Absolutionism Absolutionism marked this pull request as ready for review December 30, 2024 02:14
@sovdeeth
Copy link
Member

sovdeeth commented Feb 2, 2025

I think

do:
 ...
while:
 a
 b

is much more readable and intuitive than the current order, since it's read in order of execution

@Absolutionism Absolutionism requested review from a team as code owners April 1, 2025 04:48
@Absolutionism Absolutionism requested review from Romitou and removed request for a team April 1, 2025 04:48
@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Apr 25, 2025
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label May 15, 2025
@Absolutionism Absolutionism marked this pull request as draft June 3, 2025 17:36
@skriptlang-automation skriptlang-automation bot removed the needs reviews A PR that needs additional reviews label Jun 3, 2025
@APickledWalrus APickledWalrus linked an issue Jun 4, 2025 that may be closed by this pull request
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants