Entry Removals in json tag files

[cputnam-a11y]

Adds a fabric:remove field to JSON tag files allowing removals from existing contents.

This is useful in cases where you don't want to bring over an entire tag in a new tag, but still want the tag's contents, or in cases where gameplay logic is dictated by tags.

Such as the new Villager Trade tags, which determine which trades show up within professions and their levels. You may want to remove the enchanted book trades from Librarians, and this would offer the ability to do that in a clean and easy way.

{
  "replace": false,
  "fabric:remove": [
    "minecraft:brick"
  ],
  "values": [
    "#c:bricks", // Don't know if this exists, but it existing or not is irrelevant. Just gotta get the point across.
    "minecraft:snowball"
  ]
}

[modmuss50]

I havent done an indepth review of the impl just yet. Overall I am happy with the propsoal.

I think we should start by having this as fabric:remove. As I said on discord I am hesitant to merge a c:remove without first being 100% sure that neo are also going to adopt this, we have been burnt by this before. Lets concenrate on ourselves first as we can easily add support for c:remove later when the stars have all aligned.

Data generation support is also required for this, and I think there is a lot of scope to improve the tests.

This looks like a great start though 👍

[ChrysanthCow]

Hello, I will be taking over this PR for the time being.

I'd like to ask about changing the JSON format for this PR a bit,

I do like the simplicity of the current system, but I'd like to offer a more extensible system for any future tag related PRs, for example, ordering related fields.

My main concern is that if we keep adding new fields to the root, it leaves little room for other tag PRs to add their own contents, and makes files messier.

I'd also like to suggest a shortened format for remove entries if this is the case. !. This is always prefixed first within the tag entry, before the ID and #. No other short-forms should exist as they cannot be known at a surface level.

{
    // Default vanilla values.
    // It is preferable to use this over 'fabric:values' for any vanilla supported entries.
    // Especially when developing alongside vanilla/multiloader contexts.
    "values": [
        // This exact tag is why an ordering system for tags would be nice to have.
        "#minecraft:tooltip_order"
    ],
    // Any Fabric extended values.
    // I feel this shouldn't be put into "values" mainly to avoid potential breakage within vanilla compatible datapacks.
    // If the Conventional Tags people like this, they are free to adopt this system. 
    "fabric:values": [
        // Removes Mending from this tag, throws if the entry was never within the tag or not removed by somebody else first.
        "!minecraft:mending",
        {
            "id": "minecraft:mending",
            "fabric:remove": true
        },
        // Removes othercoolmod:frost_aspect from this tag, ignored if the content was never in the tag to begin with.
        {
            "id": "othercoolmod:frost_aspect",
            "required": false,
            "fabric:remove": true
        },
        // Order related operations.
        // No short-hand form for before/after.
        {
            "id": "mycoolmod:my_new_intro", // 
            "fabric:order": 0
        },
        {
            "id": "mycoolmod:pharaoh_curse",
            "fabric:before": [
                "#minecraft:curses"
            ]
        },
        {
            "id": "mycoolmod:frost_aspect",
            "fabric:after": [
                "minecraft:fire_aspect"
            ]
        },
        {
            "id": "mycoolmod:pharaoh_curse",
            "fabric:before": [
                "minecraft:vanishing_curse"
            ],
            "fabric:after": [
                "othercoolmod:obabo_curse"
            ]
        }
    ]
}

[ChrysanthCow]

This is probably going into mega overengineered territory, but I thought I'd suggest the full thing to see if anybody else has a better way to handle this sort of tag extension system.

[ChrysanthCow]

@cputnam-a11y Could you please change the target to 26.1 with these changes?

[ChrysanthCow]

Summary of Discord discussion with @cassiancc:

• I need to update the wording on some of the comments.
• fabric:values is better than fabric_extended_values
• Short-hand should only be used for removal operations, not order based ones.

[cputnam-a11y]

@cputnam-a11y Could you please change the target to 26.1 with these changes?

I don't have the dropdown to do so. googling suggested it must be done by someone with write perms to the target repo
(Edit Feb 8): github is just having a skill issue

[ChrysanthCow]

Honestly, I've done some thinking about this.

I think fabric:remove is fine, mainly because I doubt we'll have anybody extending fabric:values to add their own tag data.

The order stuff could be fabric:order as a different field with its own syntax instead.

[ChrysanthCow]

This should now be ready.

@cputnam-a11y You're free to make this PR a non-draft.

[cputnam-a11y]

This should now be ready.

@cputnam-a11y You're free to make this PR a non-draft.

Undrafting. Please be sure to address each unresolved review, and have the existing reviewers re-review

[ChrysanthCow]

I think I'm done now. I'm so sorry for throwing everything off with this last minute bikeshed.

[ChrysanthCow]

@cputnam-a11y Could you request re-review from ppl who have reviewed? Thanks.

[ChrysanthCow]

Failing tests are bc of outside factors atp.

[ChrysanthCow]

As the one working on this API, I think it's improved a lot with feedback in mind.

[ChrysanthCow]

No need to getOrDefault here because the the remove list is checked for isEmpty prior.

[ChrysanthCow]

PR moved over here.

[ChrysanthCow]

@cputnam-a11y Could you please close this PR. It has been superseded by the above.

[cputnam-a11y]

Succeeded by #5365