Adds a SetEffect lang util class

[TheLimeGlass]

Description

Separated the JUnit fixes in this pull request into https://github.com/SkriptLang/Skript/pull/6057 will rebase this after that gets merged

Adding my implementation of how to deal with boolean expressions not reflecting proper language statements. My addons have been using this utility class for a long time. Examples below.

What this utility class does is registers a default effect and not an expression, because returning a boolean expression should be a condition, but we still want to support changing the property value. This conversation has been brought up before in the SkUnity developer Discord channel as needing a change. So i'm gonna add my implementation that solves that said issue. Honestly this is very helpful.

/**
 * Registers an effect with patterns "set property of %type% to %boolean%" and "set %types%'[s] property to %boolean%"
 * 
 * @param effect The SetEffect class that you are registering.
 * @param property The property of the syntax.
 * @param type The type classinfo of the syntax. Can be plural.
 */
public static void register(Class<? extends SetEffect<?>> effect, String property, String type) {
	Skript.registerEffect(effect, "set " + property + " of %" + type + "% to %boolean%",
			"set %" + type + "%'[s] " + property + " to %boolean%");
}

/**
 * Registers an effect with patterns "set property of %type% to %boolean%", "set %types%'[s] property to %boolean%"
 * and "make %types% makeProperty" with "force %types% to makeProperty"
 * 
 * @param effect The SetEffect class that you are registering.
 * @param property The property of the syntax.
 * @param makeProperty The property of the syntax used for "make %types% makeProperty".
 * @param type The type classinfo of the syntax. Can be plural.
 */
public static void registerMake(Class<? extends SetEffect<?>> effect, String property, String makeProperty, String type) {
	Skript.registerEffect(effect, "set " + property + " of %" + type + "% to %boolean%",
			"set %" + type + "%'[s] " + property + " to %boolean%",
			"make %" + type + "% " + makeProperty,
			"force %" + type + "% to " + makeProperty,
			"make %" + type + "% not " + makeProperty,
			"force %" + type + "% to not " + makeProperty);
}

set visibility of player's bossbar to false
make player's bossbar visible

Examples of my addons using this utility class:

Khoryl (There are subclasses to the set effect due to how Khoryl has to operate)https://github.com/TheLimeGlass/Khoryl/tree/master/src/main/java/me/limeglass/khoryl/elements

Skellett example: https://github.com/TheLimeGlass/Skellett/blob/master/src/main/java/me/limeglass/skellett/elements/bossbars/SetBossBarVisible.java

---

Target Minecraft Versions: any Requirements: none

[bluelhf]

Would the resulting syntax for your example be force player's bossbar to visible?

[TheLimeGlass]

Would the resulting syntax for your example be force player's bossbar to visible?

Yes the word be should be included. In my test for the syntax class I use

registerMake(SetEffectTest.class, "all", "(return all|(have|be) all)", "itemtypes");

Which would make that force player's bossbar to be visible and disallow that exact calling.

[UnderscoreTud]

I, for one, think this is a totally unnecessary addition, and the implementation is a bit inflexible.

Boolean expressions seems like a reasonable approach when it comes to registering such elements. Why do they not reflect 'proper language statements'? I don't see anything wrong with:

set player's flight mode to true

... and let's assume that true is the problem: you can use on (or even yes if you feel funny). Or maybe the whole phrase sounds off? There is #4154 which adds alternatives like toggle player's flight mode. (And maybe the syntax can be revised to allow turn %booleans% (on|off)/turn %booleans% %*boolean% or something similar?) Keep in mind that this PR will pretty much invalidate that work, since you'll have to register a boolean expression for it to work.
Moreover, not registering an expression for something like this means you have to register a separate condition. I know, we do this already (even for flying), but atleast find a way to merge the two (effect & condition), so the behaviour is in one class? After all, this PR aims to also simplify creating elements like this, doesn't it? But, anyways, by using a boolean expression, you practically kill two (or even three) birds with one stone, since you can also easily pass the value to, let's say, a function. You can do:

blob({_x}'s visibility)

instead of

blob(true if {_x} is visible, else false)

(the latter, in my opinion, isn't very nice)

And let's not forget about #3838. If we are going to implement this, doesn't this get us back to where we started? I mean, I could argue that if blob({_x}'s visibility) doesn't 'reflect proper language', neither does blob({_x} is visible).

Plus, for parity with other property elements, shouldn't getExpression() be getExpr()? Could we also add a setExpr method to modify the underlying expression, as the other property elements support it, in case someone wants to override default init behaviour? Or we can change them all to getExpression & setExpression.

And is SetEffect the best name for this class? Maybe PropertyEffect like the other classes? I mean, I can see it get confused with EffChange. Do we use the same Eff* convention for classes extending this? In your addons, I see you have adopted Set* names, which, to be fair, seem like another unnecessary change in style, because the element is still an effect. We don't have separate naming styles for property expressions or conditions (we do for effect sections because they merge two different types of elements), so in the event that we go on with this, I believe we should stick to Eff* names.

I can see why and where this would be useful, but I just feel like it's not worth it. Personally, I have my reservations, but if everyone else agrees, go on!

[TheLimeGlass]

I, for one, think this is a totally unnecessary addition, and the implementation is a bit inflexible.

Boolean expressions seems like a reasonable approach when it comes to registering such elements. Why do they not reflect 'proper language statements'? I don't see anything wrong with:

set player's flight mode to true

It's not the setting changer of expressions that is the issue, it's the getting of the boolean in the expression, you should not be using an expression to return a boolean value. You should be using a condition, as you self pointed out.

... and let's assume that true is the problem: you can use on (or even yes if you feel funny). Or maybe the whole phrase sounds off? There is #4154 which adds alternatives like toggle player's flight mode. (And maybe the syntax can be revised to allow turn %booleans% (on|off)/turn %booleans% %*boolean% or something similar?) Keep in mind that this PR will pretty much invalidate that work, since you'll have to register a boolean expression for it to work. Moreover, not registering an expression for something like this means you have to register a separate condition. I know, we do this already (even for flying), but atleast find a way to merge the two (effect & condition), so the behaviour is in one class? After all, this PR aims to also simplify creating elements like this, doesn't it? But, anyways, by using a boolean expression, you practically kill two (or even three) birds with one stone, since you can also easily pass the value to, let's say, a function.

https://github.com/SkriptLang/Skript/pull/4154 is not implemented yet and could be omitted, which is why it's age is so great. A better solution to the toggle would be this pull request.

You can do:

blob({_x}'s visibility)

instead of

blob(true if {_x} is visible, else false)

(the latter, in my opinion, isn't very nice)

And let's not forget about #3838. If we are going to implement this, doesn't this get us back to where we started? I mean, I could argue that if blob({_x}'s visibility) doesn't 'reflect proper language', neither does blob({_x} is visible).

If #3838 gets merged, that'd make this pull request more reliant than registering as an expression as the conditions would be considered expressions.

Plus, for parity with other property elements, shouldn't getExpression() be getExpr()? Could we also add a setExpr method to modify the underlying expression, as the other property elements support it, in case someone wants to override default init behaviour? Or we can change them all to getExpression & setExpression.

Good point on adding the setExpression, but as for the naming, it should be it's full aliases. Changing the other method names should be done after this gets merged. It would be a large breaking change for sure, so something like a 2.9 target maybe.

And is SetEffect the best name for this class? Maybe PropertyEffect like the other classes? I mean, I can see it get confused with EffChange. Do we use the same Eff* convention for classes extending this? In your addons, I see you have adopted Set* names, which, to be fair, seem like another unnecessary change in style, because the element is still an effect. We don't have separate naming styles for property expressions or conditions (we do for effect sections because they merge two different types of elements), so in the event that we go on with this, I believe we should stick to Eff* names.

I don't see the confusion with SetEffect and EffChange, using the Set prefix makes sense and if we were to use PropertyEffect, the prefix would make it even more confusing if it was to be PropertyNameClass

I've obviously had years to play around with this utility class from my addons, and have not ran into an API issue, or even needed to override the setExpression. So I know this is the current best implementation.

[TheLimeGlass]

I'm not opposed to the change, but i'm not sure it'll be that helpful. When we discussed this on discord, my main problem was for things that don't fit the nice and neat "make" or "force" syntaxes that this creates. I think it'd be good for standardization for some expressions, but a little niche.

I really don't like the "Set" prefix idea, I think this should just be Eff. All other prefixes denote a major change in what the syntax does, and this is just an effect with some bonus registration, it should still use Eff.

That's fair, we'll have to discuss a valid prefix apon first class implementation.

[sovdeeth]

I hate to say it but I think I would have to agree with tud for the most part. I don't know if this is the best way to move forward with this kind of thing.

Having thought more about this, I'm shifting my position from "this seems unnecessary but alright" to just "this seems unnecessary". Like tud said, I think the main issue with these kinds of syntaxes is having to register both a condition and an effect, which this doesn't address.

The only real solid benefit I can see here is standardizing the syntaxes for similar properties, but it comes at the cost of being a lot less flexible. As I said in my last comment, when I had trouble with this sort of thing, it was with unusual syntax that didn't fit the kind of syntax this PR creates anyways. I just think this would be a very niche addition that doesn't really help solve the real issue of needing both a condition and effect. I also think that boolean property expressions seem perfectly fine most of the time as well.

[TheLimeGlass]

The only other way about going around with this is similar to how ExprChange was applied to every expression with an overridable method, which would potentially go into a Condition?

Proposals are needed here.

I still only see this as the most valid way to avoid the ugly line of getting boolean expressions.

[Pikachu920]

it seems like the teams consensus is that we would rather not pursue this approach. thank you for your contribution regardless