Add expressions Region Flag and Region Priority #5

DereWah · 2023-04-30T20:34:23Z

Related issue: #4

…ression

APickledWalrus

The structure here is good, just needs some cleanup. Files should also be using tabs instead of spaces.

APickledWalrus · 2023-05-02T15:26:41Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+
+    static {
+
+        Skript.registerExpression(ExprRegionFlag.class, String.class, ExpressionType.COMBINED, "[the] flag %string% of region %string% in [world] %world%");


You can use the worldguardregion classinfo here

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

APickledWalrus · 2023-05-02T17:05:11Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+
+    @Override
+    public String toString(Event arg0, boolean arg1) {
+        return "WorldGuard region flag";


the toString should represent the actual input of the user
ex: flag <flag> of <region> (take a look at some of the other syntax elements too :))

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

sovdeeth · 2023-05-02T17:12:03Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+        WorldGuard wg = WorldGuard.getInstance();
+        RegionContainer container = wg.getPlatform().getRegionContainer();


Can these be put in static fields instead of getting them each time? Not sure the details on WG's API, so just a question.

…ority

AyhamAl-Ali

Thank you for your contribution ⚡

Most of the requested changes are regarding Code Conventions
Please take your time reading them and change the code according to them.

I haven't listed all the changes needed but there are various places where my requested changes apply to other than the ones I commented on so please change them as well.

I will be reviewing again after these changes.

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

AyhamAl-Ali · 2023-05-03T15:33:14Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+    }
+
+    @Override
+    protected String[] get(Event e, WorldGuardRegion[] region) {


Use event instead of e everywhere

Suggested change

protected String[] get(Event e, WorldGuardRegion[] region) {

protected String[] get(Event event, WorldGuardRegion[] region) {

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

AyhamAl-Ali · 2023-05-03T15:36:08Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+
+
+


Suggested change

AyhamAl-Ali · 2023-05-03T15:57:08Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+import java.util.List;
+
+
+public class ExprRegionFlag extends PropertyExpression<WorldGuardRegion, String> {


Can we create a classinfo for WorldGuardFlag? if so this expr should return WorldGuardFlag classinfo instead of String

I will prepare the class to work with a ClassInfo for the Flag type then. If I have time I will create the Flag type aswell

Would setting the return type to a WorldGuardFlag classinfo mean that it will also only accept WorldGuardFlag as changers?

If so, wouldn't this make it "harder" to manage flags?

set flag "greeting" of region %worldguardregion% to %worldguardflag% looks repetitive.

Also, while creating the classinfo, what's the difference between using the normal Flag.class from WorldGuard and creating a WorldGuardClass.class with getters to access its Flag class inside?

I saw that this 2nd way is used for the WorldGuardRegion class for example. Why this double step?

No, region flags accept various types of data (boolean/string/numbers/etc.) so you may need to either accept Object.class or accept all types mentioned here

You will then have to do the proper checking and casting for change method

As for creating a wrapper class for Flag, if that helps a lot I think it's fine, but if it's not really needed then we better stick with WG Flag class

Oh I think my comment here never posted...

the WorldGuardRegion wrapper exists because ProtectedRegion seemingly does not provide any details as to the world it is located in

Gotcha, tysm guys!

I am a little unsure about a flag classinfo, mainly for the fact that it may be rather large + I'm not sure how it might handle WorldGuard addons that add new flags (e.g. defining lang entries)

AyhamAl-Ali · 2023-05-03T15:57:53Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+
+
+    @Override
+    public Class<?>[] acceptChange(final Changer.ChangeMode mode){


We don't use final modifier on method pararms anymore

Import ChangeMode directly (not static import)

Suggested change

public Class<?>[] acceptChange(final Changer.ChangeMode mode){

public Class<?>[] acceptChange(ChangeMode mode) {

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

DereWah · 2023-05-14T16:19:18Z

Anything else that should be changed?

APickledWalrus · 2023-05-14T16:43:23Z

I'll take a look this week and let you know :)

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

DereWah · 2023-05-16T17:05:00Z

Almost did all of the required changes (I still have to allow multiple regions in the Priority expression). Also, can I add to the PR the effect for deleting regions? I saw that there isn't one yet.

NotSoDelayed · 2023-05-16T17:07:42Z

The SkriptLang team loves contributions, so feel free to do so!

APickledWalrus · 2023-05-16T19:05:37Z

Almost did all of the required changes (I still have to allow multiple regions in the Priority expression). Also, can I add to the PR the effect for deleting regions? I saw that there isn't one yet.

I would do it in a separate PR if possible. Might be difficult since this PR is based on your main branch. Also, you can just add a changer to ExprRegionNamed

I will PR a change to this after Flag and Priority is done.

APickledWalrus · 2023-05-18T21:27:34Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+    public Class<? extends Flag> getReturnType() {
+        return Flag.class;
+    }


Suggested change

public Class<? extends Flag> getReturnType() {

return Flag.class;

}

public Class<Object> getReturnType() {

return Object.class;

}

APickledWalrus · 2023-05-18T21:29:42Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+
+    @Override
+    public String toString(Event event, boolean debug) {
+        return "flag " + exprFlag.toString(event, debug) + " of region " + getExpr().toString(event, debug) ;


Suggested change

return "flag " + exprFlag.toString(event, debug) + " of region " + getExpr().toString(event, debug) ;

return "the worldguard flag " + exprFlag.toString(event, debug) + " of " + getExpr().toString(event, debug) ;

region is not part of the syntax and thus not necessary

APickledWalrus · 2023-05-18T21:30:12Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+        }
+    }
+
+    public void change(Event event, Object[] delta, Changer.ChangeMode mode){


You don't need to worry about warnings IMO. You can just silently fail.

APickledWalrus · 2023-05-18T21:33:36Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+    @Override
+    protected Object[] get(Event event, WorldGuardRegion[] regions) {
+        String flag = exprFlag.getSingle(event);
+        if(flag != null) {


You should prefer to exit if the condition fails. Essentially, this helps prevent code from flying off into space with a bunch of indents. Example:

if (flag == null) return; flag.whatever();

Also, your if statements should have a space between the f and opening parenthesis )

APickledWalrus · 2023-05-18T21:34:42Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

+            case SET:
+            case ADD:
+            case REMOVE:
+                return CollectionUtils.array(Number.class);
+            case RESET:
+            case DELETE:
+                return CollectionUtils.array();


Suggested change

case SET:

case ADD:

case REMOVE:

return CollectionUtils.array(Number.class);

case RESET:

case DELETE:

return CollectionUtils.array();

case SET:

case ADD:

case REMOVE:

case RESET:

case DELETE:

return CollectionUtils.array(Number.class);

you can just combine them :)

APickledWalrus · 2023-05-18T21:37:20Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

+
+    public void change(Event event, Object[] delta, ChangeMode mode) {
+        WorldGuardRegion[] regions = getExpr().getArray(event);
+        assert regions != null;


Suggested change

assert regions != null;

shouldn't need to assert here

APickledWalrus · 2023-05-18T21:38:28Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

+    @Override
+    protected String getPropertyName() {
+        return "priority";
+    }


Suggested change

}

}

APickledWalrus · 2023-05-18T21:38:44Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionPriority.java

+import org.bukkit.event.Event;
+import org.skriptlang.skriptworldguard.worldguard.WorldGuardRegion;
+
+public class ExprRegionPriority extends SimplePropertyExpression<WorldGuardRegion, Number> {


needs documentation annotations

APickledWalrus · 2023-05-18T21:38:54Z

src/main/java/org/skriptlang/skriptworldguard/elements/expressions/ExprRegionFlag.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class ExprRegionFlag extends PropertyExpression<WorldGuardRegion, Object> {


needs documentation annotations

DereWah added 2 commits April 30, 2023 22:34

Add expression Region Flag

53991d6

Expression Region Priority

c791ad5

DereWah changed the title ~~Add expression Region Flag~~ Add expressions Region Flag and Region Priority Apr 30, 2023

AyhamAl-Ali added the feature Pull request adding a new feature. label Apr 30, 2023

DereWah and others added 3 commits May 1, 2023 09:56

Convert ExprRegionPriority from SimpleExpression to SimplePropertyExp…

090e3ac

…ression

Convert ExprRegionPriority from SimpleExpression to SimplePropertyExp…

aef2dca

…ression

Update ExprRegionPriority.java

7c3c9e9

APickledWalrus requested changes May 2, 2023

View reviewed changes

sovdeeth reviewed May 2, 2023

View reviewed changes

Convert ExprRegionFlag to a PropertyExpression, Cleanup ExprRegionPri…

f8fb772

…ority

DereWah requested a review from APickledWalrus May 3, 2023 08:50

AyhamAl-Ali suggested changes May 3, 2023

View reviewed changes

Code convention cleanup.

5781493

DereWah requested a review from AyhamAl-Ali May 5, 2023 16:58

Merge branch 'SkriptLang:main' into main

8c398e3

APickledWalrus requested changes May 16, 2023

View reviewed changes

Review fixes.

ea2cbd8

Allow multiple regions in ExprRegionPriority

4ce6e37

DereWah requested a review from APickledWalrus May 17, 2023 07:18

DereWah and others added 3 commits May 17, 2023 21:48

Allow multiple regions in ExprRegionPriority

acfb6af

Fix PropertyExpression ExprRegionFlag not registering properly.

608d261

revert ExprRegionNamed

10b10c9

I will PR a change to this after Flag and Priority is done.

APickledWalrus requested changes Jun 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expressions Region Flag and Region Priority #5

Add expressions Region Flag and Region Priority #5

DereWah commented Apr 30, 2023 •

edited by AyhamAl-Ali

APickledWalrus left a comment

APickledWalrus May 2, 2023

APickledWalrus May 2, 2023

sovdeeth May 2, 2023

AyhamAl-Ali left a comment

AyhamAl-Ali May 3, 2023

AyhamAl-Ali May 3, 2023

AyhamAl-Ali May 3, 2023

DereWah May 3, 2023

DereWah May 3, 2023

AyhamAl-Ali May 4, 2023

APickledWalrus May 4, 2023 •

edited

DereWah May 4, 2023

APickledWalrus May 5, 2023 •

edited

AyhamAl-Ali May 3, 2023 •

edited

DereWah commented May 14, 2023

APickledWalrus commented May 14, 2023

DereWah commented May 16, 2023

NotSoDelayed commented May 16, 2023

APickledWalrus commented May 16, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023

APickledWalrus May 18, 2023


		static {

		Skript.registerExpression(ExprRegionFlag.class, String.class, ExpressionType.COMBINED, "[the] flag %string% of region %string% in [world] %world%");

		WorldGuard wg = WorldGuard.getInstance();
		RegionContainer container = wg.getPlatform().getRegionContainer();

	protected String[] get(Event e, WorldGuardRegion[] region) {
	protected String[] get(Event event, WorldGuardRegion[] region) {

		import java.util.List;


		public class ExprRegionFlag extends PropertyExpression<WorldGuardRegion, String> {



		@Override
		public Class<?>[] acceptChange(final Changer.ChangeMode mode){

	public Class<?>[] acceptChange(final Changer.ChangeMode mode){
	public Class<?>[] acceptChange(ChangeMode mode) {

	return "flag " + exprFlag.toString(event, debug) + " of region " + getExpr().toString(event, debug) ;
	return "the worldguard flag " + exprFlag.toString(event, debug) + " of " + getExpr().toString(event, debug) ;

Add expressions Region Flag and Region Priority #5

Are you sure you want to change the base?

Add expressions Region Flag and Region Priority #5

Conversation

DereWah commented Apr 30, 2023 • edited by AyhamAl-Ali

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

AyhamAl-Ali 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 May 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus May 5, 2023 • edited

Choose a reason for hiding this comment

AyhamAl-Ali May 3, 2023 • edited

Choose a reason for hiding this comment

DereWah commented May 14, 2023

APickledWalrus commented May 14, 2023

DereWah commented May 16, 2023

NotSoDelayed commented May 16, 2023

APickledWalrus commented May 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DereWah commented Apr 30, 2023 •

edited by AyhamAl-Ali

APickledWalrus May 4, 2023 •

edited

APickledWalrus May 5, 2023 •

edited

AyhamAl-Ali May 3, 2023 •

edited