feat!: Force user to delete regions by name; introduce +all specifier #28

Restioson · 2024-06-11T13:05:21Z

Force the user to specify which regions they want to remove with /map region remove here <name>. This is a breaking change with regard to the command syntax, as <name> is a required parameter.
Allow most local regions to be specified by +all which includes all that the player is inside of. This is a breaking change as now region markers beginning with + are invalid.

@haykam821: this is similar to what we discussed, but instead of using @all (@ is not allowed in word string args), it uses +all, and then makes +all a reserved region name. Unfortunately I had to reintroduce this as siblings would be very messy and don't compose well at all.

Partially addresses #21

haykam821

To clarify what I discussed with you previously on Discord, we can still introduce new functionality here without removing the ability to use a certain region name. For example:

/map region remove here all
/map region remove here "all"

Using a literal and a string argument would allow simple unquoted cases to be executed under the literal node, while quoted cases would be considered under the argument node. The same principle could be applied to the custom RegionSelectorArgumentType introduced here.

Note that if we go with a design that includes reserved names, they would still be usable within map templates; they would simply not be editable with this mod.

I've left a few other considerations and smaller notes in this review.

haykam821 · 2024-06-12T16:23:31Z

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

+        }
+    }
+
+    private record RegionSelector(Either<String, SpecialRegionSelector> inner) {


Does the behavior of RegionSelector overlap with RegionPredicate?

It's a bit more specific than RegionPredicate - perhaps it could be a subclass/instance of RegionPredicate though

RegionPredicate is now 🦀

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

haykam821 · 2024-06-12T16:24:12Z

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

+        }
+    }
+
+    private record RegionSelector(Either<String, SpecialRegionSelector> inner) {


Would it make sense for RegionSelector to have multiple implementations (perhaps using a sealed interface) rather than using an Either record component?

Perhaps if it is considered to be better style - I wrote it as such as there can only be two region selector types (special or named)

If RegionSelector were made a subclass of RegionPredicate then perhaps each instance could be a different subclass? What do you think?

ok, so now that RegionPredicate is gone, how do we feel about this?

haykam821 · 2024-06-12T16:24:57Z

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

Could the region selector implementation be moved to a different package? The MapMetadataCommand class is getting a bit long.

That might be a good idea yes

It's a bit shorter now; any new thoughts? I could also extract all the static methods, but this is the only class they are used in

haykam821 · 2024-06-12T16:28:01Z

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

+    public static final DynamicCommandExceptionType INVALID_REGION_SELECTOR = new DynamicCommandExceptionType(
+            arg -> Text.stringifiedTranslatable("text.nucleoid_creator_tools.map.region.reserved_name", arg)
+    );


Could this field be named something along the lines of RESERVED_REGION_NAME to better fit the translation key (or vice versa, renaming the translation key to fit the field name and translation)?

I would go the other way and rename the key to text.nucleoid_creator_tools.map.region.selector.invalid, as it does not actually have anything to do with reserved names, but yes, they are misaligned at present.

Restioson · 2024-06-13T13:09:11Z

To clarify what I discussed with you previously on Discord, we can still introduce new functionality here without removing the ability to use a certain region name.

Don't worry - the current changeset does not add any reserved words

Gegy

Mostly nits, but haykam's comment about Vanilla client compatibility definitely needs a look 🙂

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

Gegy · 2024-06-15T11:45:40Z

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

@@ -79,25 +166,25 @@ public static void register(CommandDispatcher<ServerCommandSource> dispatcher) {
                    )))))
                    .then(literal("rename")
                        .then(literal("all")
-                            .then(argument("old", StringArgumentType.word()).suggests(regionSuggestions())
-                            .then(argument("new", StringArgumentType.word())
+                            .then(regionMarkerArg("old")


If the 'exact' marker is extracted, it might help to rename this to 'region selector'/'region predicate' kind of thing

I don't quite follow unfortunately - perhaps the attached diff is wrong though

Gegy

Looks great, awesome work! Will merge as-is and we can follow-up moving the argument handling out later if we want :)

Gegy · 2024-06-30T17:24:48Z

src/main/java/xyz/nucleoid/creator_tools/command/MapMetadataCommand.java

+        return argument(name, StringArgumentType.word()).suggests(suggestions);
+    }
+
+    @SuppressWarnings("SameParameterValue") // We want to keep this general


Nit: I believe putting this as a public helper in another class would disable that inspection - might generally be nice to have in separate classes as we are doing in Plasmid

Restioson requested review from Gegy and haykam821 June 11, 2024 13:05

Restioson force-pushed the feat/delete-by-name branch from 13a0470 to 6f1defd Compare June 11, 2024 13:10

Restioson mentioned this pull request Jun 11, 2024

feat: remove all regions by name #31

Merged

Restioson force-pushed the feat/delete-by-name branch from 6f1defd to c3ef1f3 Compare June 11, 2024 16:12

haykam821 reviewed Jun 12, 2024

View reviewed changes

Gegy requested changes Jun 15, 2024

View reviewed changes

feat!: delete region by name

2d28085

Restioson force-pushed the feat/delete-by-name branch 2 times, most recently from 15a1ce2 to 6ab4886 Compare June 16, 2024 14:39

Restioson requested a review from Gegy June 16, 2024 14:40

feat!: add special +all region selector

f864f8c

Restioson force-pushed the feat/delete-by-name branch from 6ab4886 to f864f8c Compare June 16, 2024 14:41

Restioson requested a review from haykam821 June 16, 2024 14:43

Restioson mentioned this pull request Jun 16, 2024

siege:jungle's River Gate gate-brace is too small NucleoidMC/Siege#34

Open

Gegy approved these changes Jun 30, 2024

View reviewed changes

Gegy merged commit abec0b2 into NucleoidMC:1.20.4 Jun 30, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Force user to delete regions by name; introduce +all specifier #28

feat!: Force user to delete regions by name; introduce +all specifier #28

Restioson commented Jun 11, 2024 •

edited

Loading

haykam821 left a comment

haykam821 Jun 12, 2024

Restioson Jun 13, 2024

Restioson Jun 16, 2024

haykam821 Jun 12, 2024

Restioson Jun 13, 2024

Restioson Jun 13, 2024 •

edited

Loading

Restioson Jun 16, 2024

haykam821 Jun 12, 2024

Restioson Jun 13, 2024

Restioson Jun 16, 2024

haykam821 Jun 12, 2024

Restioson Jun 13, 2024

Restioson commented Jun 13, 2024

Gegy left a comment

Gegy Jun 15, 2024

Restioson Jun 16, 2024

Gegy left a comment •

edited

Loading

Gegy Jun 30, 2024

feat!: Force user to delete regions by name; introduce +all specifier #28

feat!: Force user to delete regions by name; introduce +all specifier #28

Conversation

Restioson commented Jun 11, 2024 • edited Loading

haykam821 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

Restioson Jun 13, 2024 • edited Loading

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

Restioson commented Jun 13, 2024

Gegy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gegy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson commented Jun 11, 2024 •

edited

Loading

Restioson Jun 13, 2024 •

edited

Loading

Gegy left a comment •

edited

Loading