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

Adds a SetEffect lang util class #5219

Closed
wants to merge 29 commits into from
Closed

Conversation

TheLimeGlass
Copy link
Collaborator

@TheLimeGlass TheLimeGlass commented Nov 24, 2022

Description

Separated the JUnit fixes in this pull request into #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

@TheLimeGlass TheLimeGlass added the feature Pull request adding a new feature. label Nov 24, 2022
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
@TheLimeGlass TheLimeGlass added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Mar 17, 2023
@bluelhf
Copy link
Contributor

bluelhf commented Mar 17, 2023

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

@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Mar 17, 2023

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.

This was referenced Apr 11, 2023
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.

I would like some discussion on the usage of the BiConsumer

src/main/java/ch/njol/skript/effects/base/SetEffect.java Outdated Show resolved Hide resolved
Copy link
Member

@Pikachu920 Pikachu920 left a comment

Choose a reason for hiding this comment

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

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.

@sovdeeth
Copy link
Member

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.

@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 09:48
@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Sep 22, 2023

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.

@TheLimeGlass TheLimeGlass removed the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Sep 22, 2023
@TheLimeGlass TheLimeGlass mentioned this pull request Feb 5, 2024
1 task
@sovdeeth sovdeeth self-requested a review May 31, 2024 14:48
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.

I'm not really a fan of the set patterns here. IMO they are what we are trying to get away from when avoiding using an Effect rather than an Expression.

The make/force patterns might be a little inflexible, but it's not terrible. What might be more useful IMO is a utility that registers both an Effect and a Condition (this would have to use some sort of backing interface, would probably be a little messy).

@APickledWalrus APickledWalrus added the up for debate When the decision is yet to be debated on the issue in question label Aug 15, 2024
@sovdeeth sovdeeth removed their request for review September 13, 2024 15:13
@Fusezion Fusezion mentioned this pull request Oct 17, 2024
1 task
@Pikachu920
Copy link
Member

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

@Pikachu920 Pikachu920 closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants