DenizenScript/Denizen

Spread Around New Tag Param Usage, Static Tag Registration, new Property format, and Command autoExecute

mcmonkey4eva opened this issue · 1 comments

mcmonkey4eva commented

This is an open-ended issue intended as an easy goal for contributors to help Denizen and learn to make good PRs, without having to dive deep into complicated codework.

This comes in a few parts:

Tag Param Usage

There's a bunch of old tags that look like this:

        tagProcessor.registerStaticTag(DurationTag.class, "sub", (attribute, object) -> {
            if (!attribute.hasParam()) {
                Debug.echoError("The tag DurationTag.sub[...] must have a value.");
                return null;
            }
            return new DurationTag(object.getTicks() - attribute.paramAsType(DurationTag.class).getTicks());
        });

The attribute processing is now automated, and so the tag registration should now look like this:

        tagProcessor.registerStaticTag(DurationTag.class, DurationTag.class, "sub", (attribute, object, secondVal) -> {
            return new DurationTag(object.getTicks() - secondVal.getTicks());
        });

This exclusively applies to tags that have a MANDATORY parameter. Any tag that lacks the if (!attribute.hasParam()) { check prefix probably isn't mandatory. Check the tag meta, if the param has () on it, it's not mandatory, so don't change it.
Not all tags had echoError in them, they should still be updated to the new format.

Any PR that updates a significant number of tags to use this new format would be appreciated.

Static Tag Registration

This one requires a little bit more care. The old tag registration method was registerTag, but there is now also a registerStaticTag for when a tag is static. A 'static' tag is one in which for any given exact input, the same exact output will always result.

For example, <DurationTag.sub[<#>]> is static because the math is always the same. 5s - 3s is always 2s.
On the other hand, <util.random_boolean> is not static as whether it returns 'true' or 'false' is different every time.

Caution must be taken as many tags rely on external state factors. <world[...]> is non-static because worlds can be loaded or unloaded. LocationTag.material is non-static because somebody might break or place a block there. Anything under EntityTag is non-static as there's no such thing as a static Entity.

Mechanism Registration

Mechanisms up until 2022 were made with a matches spam method... they are now registered. All the old mechs need to be updated to the new register method.
(Note: sorta-exception for int/bool/etc. values as they need dedicated registration tools still)

Command autoExecute

Commands using the legacy parseArgs method or the interim scriptEntry.argAsX(...) style argument handling should be updated to the new autoExecute code-gen toolset.
An example commit is here: DenizenScript/Denizen-Core@ce1ba87
Some commands have arguments that are not yet compatible with the new system and should be left as-is for now.

Care should be taken to ensure the all valid argument inputs remain valid, and that default values are properly handled. In some cases, the best default is @ArgDefaultNull with an if (arg == null) { arg = something(); } check inside the method.

Commands with multiple linear arguments of previously indeterminate order should check for and show the outOfOrderArgs deprecation warning as demonstrated here: DenizenScript/Denizen-Core@b361471#diff-19ffff3dc1082c07ac26398394cc14ae15b2fb6156edcbcdbbbefe643a02dbccR58

Property Legacy Tags

Some properties (EntityTag and ItemTag properties) still use the ancient legacy attribute.startsWith style of tags. These need to be updated to modern registerTag style. Here's a (slightly outdated) example PR of this: https://github.com/DenizenScript/Denizen/pull/2334/files

Event Determinations

Update old determination handlers to new register format.

Depenizen Property Legacy Extensions

Depenizen extensions should just directly register rather than janking a property class, as seen by this upgrade example commit here: 246cb1c

Also Depenizen

Also for contributors looking for things to do, if you use Depenizen, consider updating the very old legacy Depenizen code - startsWith and other legacy formats are used throughout.

Generally speaking, the priority order is:

  • Denizen Core (most important, needs to be kept clean and modern)
  • Denizen
  • dDiscordBot
  • Frequently used Depenizen modules
  • Unpopular Depenizen modules (least important, might not even make a difference if changed anyway)
mcmonkey4eva commented

Property Format

Here's the changes needed to update a property to modern format:

  • Fix misleading registered lambda input names (eg material or object should be prop)
  • remove getFrom copypasta
  • simplify describes code - input param as correct type instead of ObjectTag, clean modern code
  • Remove constructor if possible, or simplify it if it's needed elsewhere (as seen in MaterialAge)
  • remove redundant field (in favor of built-in object field)
  • use new convenience methods (eg prop.getItemMeta() instead of prop.object.getItemMeta()) where applicable
  • move legacy adjusts mechs to modern register format
  • remove handledTags/handledMechs
  • change implements Property to eg extends MaterialProperty (distinct per object type)
  • use getPropertyValue instead of getPropertyString
  • make sure @override isn't missing from getPropertyId or getPropertyValue
  • remove isX/getX method spam in favor of modern Java if (val instanceof Type newVal) {
  • Other modernizations as relevant, eg asEnum method
  • make sure to remove unused imports
  • Some backversion compats can actually be removed, eg the commented out getX methods, as the modern equivalent does not break on old versions
  • make sure the order of methods is exactly describes, getPropertyValue, getPropertyId, (other methods as relevant), register, (other methods as relevant)
  • test the properties you change, including tag/mech and the property output (for EntityTag use .describe, for others just narrate the object)
  • if they have version-specific code, test on an older minecraft version (mainly looking for explosive errors, Java sometimes gets unhappy about version-specific code)
  • TradeTag and InventoryTag have been done in full already. MaterialTag is priority, ItemTag and EntityTag need updates too
  • I have done a few each from each of the required types to demonstrate the changes: c28dede...ecf8b62

Part 2:

  • Commit demonstrating part 2: 0559148
  • Set the data type in the class declaration, eg the MapTag in extends ItemProperty<MapTag>
  • add public void setPropertyValue(DATA_TYPE_HERE param, Mechanism mechanism) as the mech impl
  • Copy the mech meta
    • change it to <--[property]
    • put it at the top of the file
    • Remove the @tags
      • If there's extra tags linked outside the property tag, swap it to an @link
    • Adjust wording to fit the fact that the description is for both the tag and mech now (eg sets -> controls)
  • remove the tag meta
  • add autoRegister("NAME_HERE", CLASS_HERE.class, TYPE_HERE.class, false);
    • If there was "registerStaticTag" previously, change false to true (static)
  • if the tag always returns a value but the getPropertyString/Value returns null sometimes, implement isDefaultValue to indicate the null states (return true if the value should be null).
  • clean up the imports after
  • any other file cleanups as necessary (eg merging together now-redundant methods)

Note: some properties can not be easily updated, if eg they have inconsistent tag/mech names, or weird legacy subtag logic, or...